[vtk-developers] vtkDataArray performance; vtkErrorMacro semantics

Steve Pieper pieper at isomics.com
Tue Apr 26 19:34:48 EDT 2016


Please also keep in mind the interactive python use case (like ipython or a
slicer console) where you are exploring the API or testing code.  A
vtkErrorMacro message is very helpful.  But an assert will either just bomb
out if you are using a debug build or possibly lead to undefined behavior
in a release build.

-Steve

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
>
>
>
> _______________________________________________
> 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/20160426/c4ca57f7/attachment.html>


More information about the vtk-developers mailing list