[vtk-developers] Proposal to add "vtkInfoMacro"

Sat Dec 13 15:42:45 EST 2014

You are right, it’s not necessary to make this additional logging macro available for core VTK library modules. It can be anywhere in VTK where VTK tests, examples, and VTK-based applications can access it. Do you have any suggestion for a good place?

Having the macro in VTK allows a standardization on the macro name, parameters, and behavior among all VTK-based applications. The macro would be used in application classes derived from vtkObject.

Andras

From: Berk Geveci [mailto:berk.geveci at kitware.com]
Sent: Saturday, December 13, 2014 15:16
To: Andras Lasso
Cc: David Cole; David Gobbi; Jean-Christophe Fillion-Robin (JChris.FillionR at kitware.com); vtk-developers at vtk.org
Subject: Re: [vtk-developers] Proposal to add "vtkInfoMacro"

Sorry but I remain unconvinced. I haven't heard a compelling reason why this functionality should be available to core VTK modules and why the maintainers of those modules need to worry about it. So far, the only example is for a class in Slicer.

Whether this is in a separate header or not, putting it in Common/Core will make it possible for all modules in VTK to use this functionality without any compile errors. As I mentioned, I'd like to see functionality migrate out of Common/Core to outer modules not vice versa. "There are other application-only functions in Common already" is not a good reason why we should cram more in there. Also, vtkOutputWindow is used for VTK-level functionality such as debugging and error reporting. Similar to vtkDebugLeaks. I wouldn't call that application specific.

Best,
-berk

On Sat, Dec 13, 2014 at 11:35 AM, Andras Lasso <lasso at queensu.ca<mailto:lasso at queensu.ca>> wrote:
Yes, sure we don’t need VTK to include a complete logging framework such as log4cpp. We just need a convenient mechanism to send messages to logging frameworks from VTK.

What do you think about the following?

* Allow completely disable vtkInfoMacro by an #ifdef (for example, if  #ifndef VTK_INFO_ENABLED then vtkInfoMacro is an empty statement; we can set VTK_INFO_ENABLED to false by default)

* Put the macro in a separate header in Common/vtkInfo.h to not make the macro visible to VTK classes by default. GUISupport directory would be too restrictive, as logging is not related to GUI and there are other application-only functions in Common already (such as vtkOutputWindow::DisplayText), but any other suggestion is welcome.

Andras

From: David Cole [mailto:DLRdave at aol.com<mailto:DLRdave at aol.com>]
Sent: Saturday, December 13, 2014 09:28
To: David Gobbi
Cc: Berk Geveci; vtk-developers at vtk.org<mailto:vtk-developers at vtk.org>; Andras Lasso

Subject: Re: [vtk-developers] Proposal to add "vtkInfoMacro"

I agree with Berk. VTK should NOT include the kitchen sink... This belongs elsewhere -- there are whole separate libraries that are actually focused on logging, and doing it in a high performance, high functionality manner. log4cpp, g2log, and boost all come to mind.

Great to hear talk of testing performance regressions on the dashboard -- looking forward to the day that's a reality.

Thx,
David C.

On Friday, December 12, 2014, David Gobbi <david.gobbi at gmail.com<mailto:david.gobbi at gmail.com>> wrote:
Well, vtkImageReslice is probably not the best "speed" example any more, it has become fat with all the features that I've added to it over the years :)   And in the old days, a big reason for its speed was that it did some bit-twiddling to work around the disastrously slow floor() operations on old CPUs.

However, it is certainly true with modern computer architectures, with their multiple cores and fast caches, doing _any_ application-level or system-level operations within the algorithmic code causes a significant performance hit, because that takes the CPU core out of the "zone" where it can work at its full potential.

 - David

On Fri, Dec 12, 2014 at 5:12 PM, Berk Geveci <berk.geveci at kitware.com<mailto:berk.geveci at kitware.com>> wrote:
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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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/20141213/c3296e99/attachment-0001.html>