<div dir="ltr">So we don't duplicate effort, Berk and I talked about this a bit and I'll push a change that moves the implementation for this method into the implementation classes to reduce dispatch overhead. I'll leave the checks as-is for now.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 28, 2016 at 1:19 PM, Berk Geveci <span dir="ltr"><<a href="mailto:berk.geveci@kitware.com" target="_blank">berk.geveci@kitware.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Yeah but that's kicking the can to another method :-) Should we maintain a whole API for testing/debugging and then another for fast access? What guarantees then that folks will rewrite their filters/code to use the faster APIs after testing/debugging? As it is proven by the current set of VTK filters, people use whatever is convenient in writing their current filter and don't necessarily dig deep into what is the most efficient.<div><br></div><div>I do understand the desire to do boundary checking etc. though. But then compiling in debug mode seems to be a decent compromise. I am somewhat on the fence on whether assert() or error macro is better in that case. It would be fairly easy to define the error macro such that it optionally asserts so maybe that's the way to go...</div><div><br></div><div>In core data structures such as arrays and datasets, I'd like to see us moving in a direction that makes it easier to write performant and thread safe code. With the current set of methods, this is very hard to achieve because of its sprawling nature.</div><div><br></div><div>By the way, the method in discussion does not do any boundary checking. Similar to other SetTuple() methods. InsertTuple() methods do boundary checking. In general, SetTuple() methods do not provide a safety net against developer errors for performance. For example:</div><div><br></div><div><div><font face="monospace, monospace">template <class T></font></div><div><font face="monospace, monospace">void vtkDataArrayTemplate<T>::SetTuple(vtkIdType i, const float* tuple)</font></div><div><font face="monospace, monospace">{</font></div><div><font face="monospace, monospace"> vtkIdType loc = i * this->NumberOfComponents;</font></div><div><font face="monospace, monospace"> for(int j=0; j < this->NumberOfComponents; ++j)</font></div><div><font face="monospace, monospace"> {</font></div><div><font face="monospace, monospace"> this->Array[loc+j] = static_cast<T>(tuple[j]);</font></div><div><font face="monospace, monospace"> }</font></div><div><font face="monospace, monospace"> this->DataChanged();</font></div><div><font face="monospace, monospace">}</font></div></div><div><font face="monospace, monospace"><br></font></div><div><font face="arial, helvetica, sans-serif">Obviously, there are many things that can wrong here. So, adding more checks that are debug mode only could actually make things safer overall and help us catch boundary condition issues.</font></div><div><br></div><div><font face="arial, helvetica, sans-serif">Best,</font></div><div><font face="arial, helvetica, sans-serif">-berk</font></div><div><font face="arial, helvetica, sans-serif"><br></font></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Apr 26, 2016 at 4:46 PM, Andras Lasso <span dir="ltr"><<a href="mailto:lasso@queensu.ca" target="_blank">lasso@queensu.ca</a>></span> wrote:<br></span><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">It’s great to have these safe get/set methods for testing and debugging. Please don’t remove them or the boundary checks.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">It would be useful to add to the documentation that these methods are not intended for bulk data access and what methods should be used instead.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Andras<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> vtk-developers [mailto:<a href="mailto:vtk-developers-bounces@vtk.org" target="_blank">vtk-developers-bounces@vtk.org</a>]
<b>On Behalf Of </b>Berk Geveci<br>
<b>Sent:</b> Tuesday, April 26, 2016 4:26 PM<br>
<b>To:</b> David Lonie <<a href="mailto:david.lonie@kitware.com" target="_blank">david.lonie@kitware.com</a>><br>
<b>Cc:</b> VTK Developers <<a href="mailto:vtk-developers@vtk.org" target="_blank">vtk-developers@vtk.org</a>><br>
<b>Subject:</b> Re: [vtk-developers] vtkDataArray performance; vtkErrorMacro semantics<u></u><u></u></span></p><div><div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">I agree with Dave Lonie's later statement. At least, let's focus on removing the use of that method and others like it that are horrible. Also, having these methods do the dispatch in the superclass rather than being virtual is a horrible
performance drag. I recently instrumented an implementation of the contour filter that spent 75% of its time in SetTuples() (yikes!).<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">My suggestion would be to switch to a virtual call that assumes input & output arrays are of the same type and leaving it up to the filter to make sure that the types match once. <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">If it is easier in the short term, I am fine with by making those checks conditional to DEBUG or replacing them with some sort of assert(). However, you are still leaving a lot of performance on the floor by keeping that dispatch there...<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Best,<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">-berk<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Tue, Apr 26, 2016 at 3:26 PM, David Lonie <<a href="mailto:david.lonie@kitware.com" target="_blank">david.lonie@kitware.com</a>> wrote:<u></u><u></u></p>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<div>
<p class="MsoNormal">On Tue, Apr 26, 2016 at 2:58 PM, Sean McBride <<a href="mailto:sean@rogue-research.com" target="_blank">sean@rogue-research.com</a>> wrote:<u></u><u></u></p>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Hi all,<br>
<br>
So we're continuing to profile our app to improve the performance of something, and I have another vtkDataArray question. This method:<br>
<br>
void vtkDataArray::SetTuple(vtkIdType dstTupleIdx, vtkIdType srcTupleIdx,<br>
vtkAbstractArray *source)<br>
<br>
starts with 3 checks. If they fail, it uses vtkErrorMacro and bails.<br>
<br>
First, what are the semantics of hitting vtkErrorMacro? Is it meant to be like an assertion, ie something that should be impossible, and therefore useful in debug but discardable in release?<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I've often wondered this myself, I'm not aware of an 'official' interpretation of these. From context, it seems to usually be used to enforce a documented/logical constraint. I often use them as non-fatal asserts.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I know the dashboards are configured to treat any output coming from vtkErrorMacro as a test failure.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">We've added assert(0) in the three branches, and our code never hits them and no VTK unit test does either.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Not surprising -- the method does nothing (and tests will fail) if it hits them :)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">The 3 checks are individually fast, but the method is called so very very often that removing the 3 checks entirely gives us a full 20% runtime speedup of Render(), so I'm hoping it would be acceptable to wrap them in #ifndef NDEBUG, or
otherwise remove them from release builds.<u></u><u></u></p>
</blockquote>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">+1 to the idea. I'd love to see this wrapped up in a vtkAssertMacro or similar, as well as some clarification on the semantics of Warning/ErrorMacro. I think there was some discussion started by Kyle Lutz a few years back about whether
or not to just use asserts in these situations, and there was some controversy around the idea and he abandoned the proposal in the end. I can't remember or imagine why people objected at the moment, but the thread should be in the archives.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I'd actually prefer to remove that (and similar) methods completely (settle down folks, I'm not actually proposing this! I've learned to just accept these warts and not try to fix problematic APIs in VTK). They have performance problems
beyond the sanity checks, the repeated dispatch calls add up, too. It's too fine-grained an action for the expense it occurs. It used to be worse, actually -- back when I was profiling some of these functions the vast majority of CPU time was spent in strcmp,
because SafeDownCast was being used to test if the source arrays was a data array. I believe it was actually this method that made me add the vtkAbstractArray::FastDownCast system.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Dave<u></u><u></u></p>
</div>
</div>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
_______________________________________________<br>
Powered by <a href="http://www.kitware.com" target="_blank">www.kitware.com</a><br>
<br>
Visit other Kitware open-source projects at <a href="http://www.kitware.com/opensource/opensource.html" target="_blank">
http://www.kitware.com/opensource/opensource.html</a><br>
<br>
Search the list archives at: <a href="http://markmail.org/search/?q=vtk-developers" target="_blank">
http://markmail.org/search/?q=vtk-developers</a><br>
<br>
Follow this link to subscribe/unsubscribe:<br>
<a href="http://public.kitware.com/mailman/listinfo/vtk-developers" target="_blank">http://public.kitware.com/mailman/listinfo/vtk-developers</a><br>
<br>
<u></u><u></u></p>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div></div></div>
</div>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div>