[vtk-developers] vtkDataArray performance; vtkErrorMacro semantics

Thu Apr 28 13:49:38 EDT 2016

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.

On Thu, Apr 28, 2016 at 1:19 PM, Berk Geveci <berk.geveci at kitware.com>
wrote:

> 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.
>
> 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...
>
> 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.
>
> 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:
>
> template <class T>
> void vtkDataArrayTemplate<T>::SetTuple(vtkIdType i, const float* tuple)
> {
>   vtkIdType loc = i * this->NumberOfComponents;
>   for(int j=0; j < this->NumberOfComponents; ++j)
>     {
>     this->Array[loc+j] = static_cast<T>(tuple[j]);
>     }
>   this->DataChanged();
> }
>
> 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.
>
> Best,
> -berk
>
>
>
> On Tue, Apr 26, 2016 at 4:46 PM, Andras Lasso <lasso at queensu.ca> wrote:
>
>> It’s great to have these safe get/set methods for testing and debugging.
>> Please don’t remove them or the boundary checks.
>>
>>
>>
>> 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.
>>
>>
>>
>> Andras
>>
>>
>>
>> *From:* vtk-developers [mailto:vtk-developers-bounces at vtk.org] *On
>> Behalf Of *Berk Geveci
>> *Sent:* Tuesday, April 26, 2016 4:26 PM
>> *To:* David Lonie <david.lonie at kitware.com>
>> *Cc:* VTK Developers <vtk-developers at vtk.org>
>> *Subject:* Re: [vtk-developers] vtkDataArray performance; vtkErrorMacro
>> semantics
>>
>>
>>
>> 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!).
>>
>>
>>
>> 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.
>>
>>
>>
>> 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...
>>
>>
>>
>> Best,
>>
>> -berk
>>
>>
>>
>>
>>
>> On Tue, Apr 26, 2016 at 3:26 PM, David Lonie <david.lonie at kitware.com>
>> wrote:
>>
>> On Tue, Apr 26, 2016 at 2:58 PM, Sean McBride <sean at rogue-research.com>
>> wrote:
>>
>> Hi all,
>>
>> So we're continuing to profile our app to improve the performance of
>> something, and I have another vtkDataArray question.  This method:
>>
>> void vtkDataArray::SetTuple(vtkIdType dstTupleIdx, vtkIdType srcTupleIdx,
>>                             vtkAbstractArray *source)
>>
>> starts with 3 checks.  If they fail, it uses vtkErrorMacro and bails.
>>
>> 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?
>>
>>
>>
>> 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.
>>
>>
>>
>> I know the dashboards are configured to treat any output coming from
>> vtkErrorMacro as a test failure.
>>
>>
>>
>> We've added assert(0) in the three branches, and our code never hits them
>> and no VTK unit test does either.
>>
>>
>>
>> Not surprising -- the method does nothing (and tests will fail) if it
>> hits them :)
>>
>>
>>
>> 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.
>>
>>
>>
>> +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.
>>
>>
>>
>> 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.
>>
>>
>>
>> Dave
>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>>
>> Follow this link to subscribe/unsubscribe:
>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20160428/39dbadc3/attachment.html>