[vtk-developers] Proposal to add "vtkInfoMacro"

[Berk Geveci]

I'd be fine with it if the header is put into a higher level module such as
GUISupport and has some examples demonstrating its purpose and some
documentation providing a guide on how to use it. I would not agree to
putting this at a place where it may encourage folks to use it in
algorithms and other core functionality. As I mentioned previously, I
already have the unpleasant job of ripping out a number of "convenience"
functionality that are going to have significant impact on performance as
we continue to multi-thread more algorithms. All of these features look
harmless when initially added. Then they find their way into core
functionality slowly but surely. When you are considering 200-way
parallelism (which we are, since we are porting to Intel Xeon Phi already),
any little bit makes a difference. Some simple examples:

* Garbage collection
* vtkDataArrayTemplate::DataChanged()
* Abundant use of Modified()
* Progress reporting

Very nice and convenient feature. They may lead to a code running more
slowly as more threads are added rather than scale up.

I don't agree that misuse of such features would get caught in reviews.
Very few reviewers are conscious of potential performance issues. Because
very few developers are performance conscious at this point (Dave Gobbi
excepted, he writes amazingly fast code, see image reslice).

So this is not the time to pepper core classes with convenience
functionality. It is time to simplify and come with very clear guidelines
on what core classes and algorithms can and cannot do. Once the development
community becomes more aware of these issues and our dashboards are able to
catch performance regressions, we can relax some.

Best,
-berk




On Fri, Dec 12, 2014 at 9:52 AM, David Gobbi <david.gobbi at gmail.com> wrote:
>
> However, I have reservations about putting this macro in vtkSetGet.h. If
> it goes in its own header, then the chances of it being confused with
> vtkDebugMacro are significantly reduced.
>
>  - David
>
>
> On Fri, Dec 12, 2014 at 7:24 AM, Andras Lasso <lasso at queensu.ca> wrote:
>
>>  I completely agree with David. This is an application support feature.
>> There are several examples for such features in VTK, including QVTKWidget
>> or vtkOutputWindow::DisplayText(). These are not meant to be used in VTK
>> library itself (other than examples and tests) but in VTK-based
>> applications. They make the life of application developers easier, allow
>> standardization, reduces code/feature duplication.
>>
>>
>>
>> Adding the macro has negligible impact on VTK but would help application
>> developers.
>>
>>
>>
>> Andras
>>
>>
>>
>>
>>
>> *From:* David Gobbi [mailto:david.gobbi at gmail.com]
>> *Sent:* Friday, December 12, 2014 09:16
>> *To:* vtk-developers at vtk.org
>> *Cc:* Jean-Christophe Fillion-Robin; Berk Geveci; Andras Lasso
>> *Subject:* Re: [vtk-developers] Proposal to add "vtkInfoMacro"
>>
>>
>>
>> On the one hand, I can see that it is useful to have a message macro that
>> doesn't require a debug build.  In my own apps, I often write app-specific
>> classes that are derived from vtkObject and this is a macro that I might
>> use from time to time.  Unlike Berk, I'm not all that worried that it might
>> be abused.  For VTK, we have code review.
>>
>>
>>
>> On the other hand, I don't think we have a use case for this within VTK
>> itself, it would be there purely to serve external VTK classes and apps.
>> And people who need this feature could simply put this macro (or a similar
>> one) in their own header files.
>>
>>
>>
>> Summary: this is a convenience feature, IMHO a mostly harmless one, and
>> one that I might use.
>>
>>
>>
>>  - David
>>
>>
>>
>>
>>
>> On Fri, Dec 12, 2014 at 6:27 AM, Berk Geveci <berk.geveci at kitware.com>
>> wrote:
>>
>>  I disagree with this change. There is no compelling VTK specific
>> example for why this is needed. The examples are all from Slicer - which to
>> me says that they can be implemented in Slicer.
>>
>>
>>
>> Adding a logging functionality without clear guidelines on how to use it
>> is dangerous. It can lead to folks using it for debugging in performance
>> critical sections and since it is not compiled out in Release builds, it
>> can lead to significant performance issues, specially in multi-threaded
>> code. As it is, there are lot of minor issues like this that we will have
>> to go and clean up (progress being one of them).
>>
>>
>>
>> The main use case seems to be tracking interaction/workflow changes. My
>> suggestion is to employ a method appropriate to that. For example, events
>> and listeners.
>>
>>
>>
>> Best,
>>
>> -berk
>>
>>
>>
>>
>>
>> On Thu, Dec 11, 2014 at 12:06 PM, Jean-Christophe Fillion-Robin <
>> jchris.fillionr at kitware.com> wrote:
>>
>>     Hi Folks,
>>
>> While developing 3D Slicer, we created a macro named
>>
>>                                           vtkInfoMacro
>>
>> similar to "vtkErrorMacro/vtkDebugMacro/vtkWarningMacro".
>>
>>   We would like to contribute it back to VTK core.
>>
>>
>>
>>  The associated topic is:
>>
>>                       http://review.source.kitware.com/#/c/18385/
>>
>>  It would be great to get feedback before moving forward.
>>
>> Thanks
>>
>> Jc
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/vtk-developers/attachments/20141212/2f19c884/attachment.html>