Proposal: Simplify vtkDebugLeaks registration

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposal: Simplify vtkDebugLeaks registration

David Lonie-2
Hi all,

We've been seeing some issues surrounding the use of vtkDebugLeaks and
template classes lately. The main problem stems from the difficulties
of ensuring that ::GetClassName() returns a unique string for
different instantiations of a templated object. This is important to
ensure that things like SafeDownCast work correctly.

At the moment, ::GetClassName() returns typeid(this).name() for
templated types, which is typically very different from the string
passed into vtkObjectFactory::ConstructInstance(...) by the object
factory macros used to implement the ::New() methods.

Right now we jump through some hoops to try to make sure that
vtkDebugLeaks::ConstructClass is called with the ::GetClassName()
string, since that is used when vtkObjectBase::UnregisterInternal
removes an object from the leak tracker. But we've missed some spots
and there are quite a few classes that don't bother with the macros,
and thus have custom implementations of ::New() that need to
explicitly register themselves with DebugLeaks. It's getting messy and
problematic trying to keep all of this consistent.

I propose that we simplify DebugLeaks to make the registration happen
automatically. I see a couple of ways to do this:

1) Just track the objects directly by registering their addresses in
vtkObjectBase's constructor. This would increase the memory footprint
of vtkDebugLeaks, but I think that's not a huge issue since
vtkDebugLeaks is a testing/development tool that isn't used in
production AFAIK. This would also allow a more detailed printout of
the leaked objects since we could call Print() on them.

2) Continue registering name strings, but do so from a
vtkObjectBase::InternalInitialize() (or similar) method that calls
vtkDebugLeaks::ConstructClass(this->GetClassName(). This would still
need to be called explicitly in custom ::New() implementations, but it
would be easier to maintain, update, and keep consistent/correct
thanks to the encapsulation to a single method.

Note that we can't just call
vtkDebugLeaks::ConstructClass(this->GetClassName()) from
vtkObjectBase's constructor due to how virtual methods in constructors
work. ::GetClassName() will always return "vtkObjectBase" when called
from vtkObjectBase's constructor.

Any thoughts/objections/suggestions?

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

Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Simplify vtkDebugLeaks registration

Moreland, Kenneth
Why not use typeid(this) instead of GetClassName? As long as this has one virtual function, I believe typeid will give the id for the base class.

-Ken

-----Original Message-----
From: vtk-developers [mailto:[hidden email]] On Behalf Of David Lonie
Sent: Thursday, September 8, 2016 3:42 PM
To: VTK Developers <[hidden email]>
Subject: [EXTERNAL] [vtk-developers] Proposal: Simplify vtkDebugLeaks registration

Hi all,

We've been seeing some issues surrounding the use of vtkDebugLeaks and template classes lately. The main problem stems from the difficulties of ensuring that ::GetClassName() returns a unique string for different instantiations of a templated object. This is important to ensure that things like SafeDownCast work correctly.

At the moment, ::GetClassName() returns typeid(this).name() for templated types, which is typically very different from the string passed into vtkObjectFactory::ConstructInstance(...) by the object factory macros used to implement the ::New() methods.

Right now we jump through some hoops to try to make sure that vtkDebugLeaks::ConstructClass is called with the ::GetClassName() string, since that is used when vtkObjectBase::UnregisterInternal removes an object from the leak tracker. But we've missed some spots and there are quite a few classes that don't bother with the macros, and thus have custom implementations of ::New() that need to explicitly register themselves with DebugLeaks. It's getting messy and problematic trying to keep all of this consistent.

I propose that we simplify DebugLeaks to make the registration happen automatically. I see a couple of ways to do this:

1) Just track the objects directly by registering their addresses in vtkObjectBase's constructor. This would increase the memory footprint of vtkDebugLeaks, but I think that's not a huge issue since vtkDebugLeaks is a testing/development tool that isn't used in production AFAIK. This would also allow a more detailed printout of the leaked objects since we could call Print() on them.

2) Continue registering name strings, but do so from a
vtkObjectBase::InternalInitialize() (or similar) method that calls vtkDebugLeaks::ConstructClass(this->GetClassName(). This would still need to be called explicitly in custom ::New() implementations, but it would be easier to maintain, update, and keep consistent/correct thanks to the encapsulation to a single method.

Note that we can't just call
vtkDebugLeaks::ConstructClass(this->GetClassName()) from vtkObjectBase's constructor due to how virtual methods in constructors work. ::GetClassName() will always return "vtkObjectBase" when called from vtkObjectBase's constructor.

Any thoughts/objections/suggestions?

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

Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Simplify vtkDebugLeaks registration

VTK - Dev mailing list
A few comments:

That should be typeid(*this), and I'm uncertain if that works in the
context of a base class constructor before the derived class's
constructor is called. Does it? Doesn't seem like it should...

If you do end up tracking every instance, and printing them all at
leak time, perhaps have a sane way of only printing **some** of them.
Usually, when I end up with a leak, it's connected to a vast network
of objects, and they all leak, with hundreds, if not thousands of
objects leaking.

Also, if you end up tracking every instance, it would be great to have
an API to the leaks manager to count and iterate all the outstanding
instances.


2 cents,
David C.



On Thu, Sep 8, 2016 at 5:53 PM, Moreland, Kenneth <[hidden email]> wrote:

> Why not use typeid(this) instead of GetClassName? As long as this has one virtual function, I believe typeid will give the id for the base class.
>
> -Ken
>
> -----Original Message-----
> From: vtk-developers [mailto:[hidden email]] On Behalf Of David Lonie
> Sent: Thursday, September 8, 2016 3:42 PM
> To: VTK Developers <[hidden email]>
> Subject: [EXTERNAL] [vtk-developers] Proposal: Simplify vtkDebugLeaks registration
>
> Hi all,
>
> We've been seeing some issues surrounding the use of vtkDebugLeaks and template classes lately. The main problem stems from the difficulties of ensuring that ::GetClassName() returns a unique string for different instantiations of a templated object. This is important to ensure that things like SafeDownCast work correctly.
>
> At the moment, ::GetClassName() returns typeid(this).name() for templated types, which is typically very different from the string passed into vtkObjectFactory::ConstructInstance(...) by the object factory macros used to implement the ::New() methods.
>
> Right now we jump through some hoops to try to make sure that vtkDebugLeaks::ConstructClass is called with the ::GetClassName() string, since that is used when vtkObjectBase::UnregisterInternal removes an object from the leak tracker. But we've missed some spots and there are quite a few classes that don't bother with the macros, and thus have custom implementations of ::New() that need to explicitly register themselves with DebugLeaks. It's getting messy and problematic trying to keep all of this consistent.
>
> I propose that we simplify DebugLeaks to make the registration happen automatically. I see a couple of ways to do this:
>
> 1) Just track the objects directly by registering their addresses in vtkObjectBase's constructor. This would increase the memory footprint of vtkDebugLeaks, but I think that's not a huge issue since vtkDebugLeaks is a testing/development tool that isn't used in production AFAIK. This would also allow a more detailed printout of the leaked objects since we could call Print() on them.
>
> 2) Continue registering name strings, but do so from a
> vtkObjectBase::InternalInitialize() (or similar) method that calls vtkDebugLeaks::ConstructClass(this->GetClassName(). This would still need to be called explicitly in custom ::New() implementations, but it would be easier to maintain, update, and keep consistent/correct thanks to the encapsulation to a single method.
>
> Note that we can't just call
> vtkDebugLeaks::ConstructClass(this->GetClassName()) from vtkObjectBase's constructor due to how virtual methods in constructors work. ::GetClassName() will always return "vtkObjectBase" when called from vtkObjectBase's constructor.
>
> Any thoughts/objections/suggestions?
>
> 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
>
_______________________________________________
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

Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Simplify vtkDebugLeaks registration

David Lonie-2
On Thu, Sep 8, 2016 at 6:11 PM, David Cole <[hidden email]> wrote:
> A few comments:
>
> That should be typeid(*this), and I'm uncertain if that works in the
> context of a base class constructor before the derived class's
> constructor is called. Does it? Doesn't seem like it should...

typeid will always resolve to the "current" class when called from a
constructor/destructor.

For science: http://codepad.org/WWweG8hk

#include <iostream>

struct Base
{
  Base() { std::cout << typeid(*this).name() << std::endl; }
  virtual ~Base() { std::cout << "~" << typeid(*this).name() << std::endl; }

  void Init() const { std::cout << typeid(*this).name() << std::endl; }
};

struct Derived : public Base
{
};

int main()
{
  Derived d;
  d.Init();
  return 0;
}

Output:
4Base
7Derived
~4Base

The second suggestion in my first email would move the construction
logic to something like the Init() method in that example, which
resolves properly to the Derived class.

> If you do end up tracking every instance, and printing them all at
> leak time, perhaps have a sane way of only printing **some** of them.
> Usually, when I end up with a leak, it's connected to a vast network
> of objects, and they all leak, with hundreds, if not thousands of
> objects leaking.

I agree, digging through the ::Print() output for 100's of objects
would not be useful for most cases! My plan for the object-based
report would be to iterate through the leaked objects, collect
classnames/counts, and print the summary just like it does now by
default. Optionally it could check an environment variable to print a
complete output with object details.

> Also, if you end up tracking every instance, it would be great to have
> an API to the leaks manager to count and iterate all the outstanding
> instances.

That would certainly be useful. Something like void
vtkDebugLeaks::GetCurrentObjects(vtkObjectBaseCollection *col) should
do the trick.

Personally I'm leaning towards the second option (collect strings but
use a centralized vtkObjectBase::InternalInit() method) as it's less
intrusive/less work.

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

Reply | Threaded
Open this post in threaded view
|

Re: Proposal: Simplify vtkDebugLeaks registration

David Lonie-2
I've pushed a WIP for this to https://gitlab.kitware.com/vtk/vtk/merge_requests/1982. Reviewers needed.

In a nutshell, all vtkDebugLeaks registration now occurs in a new method vtkObjectBase::InitializeObjectBase, and *nowhere else*. This is called after object construction. The basic rule of thumb is "if you call new (the C++ operator, not the vtk static New() pattern), call InitializeObjectBase()". This ensures that all vtkObjectBase subclasses, including templates, will be registered / deregistered from vtkDebugLeaks consistently and removes the need to scatter "#ifdef VTK_DEBUG_LEAKS" blocks around. The vtkObjectFactory macros all take care of this for you, but if you write a custom ::New() implementation, you'll need to make sure to call this.

The vtkObjectFactory macros that will handle vtkDebugLeaks registration are:

// Simply "return new thisClass;", unless
// VTK_ALL_NEW_OBJECT_FACTORY is defined, then
// use VTK_OBJECT_FACTORY_NEW_BODY to implement.
vtkStandardNewMacro(thisClass)
VTK_STANDARD_NEW_BODY(thisClass)

// Check object factory for override, return new thisClass if none found.
vtkObjectFactoryNewMacro(thisClass)
VTK_OBJECT_FACTORY_NEW_BODY(thisClass)

// Check object factory for override, print error message if none found.
vtkAbstractObjectFactoryNewMacro(thisClass)
VTK_ABSTRACT_OBJECT_FACTORY_NEW_BODY(thisClass)

In addition, all objects returned from vtkObjectFactory::CreateInstance will already be registered with vtkDebugLeaks.

There are two pre-existing exceptions to vtkDebugLeaks -- vtkCommand subclasses (all are registered as "vtkCommand or subclass" in the vtkCommand constructor), and vtkInformationKey subclasses (These are static objects that aren't reference counted). The patterns used for these classes prior to this change will still work afterwards.

I also added a preprocessor define to vtkObjectBase.h: VTK_HAS_INITIALIZE_OBJECT_BASE. You can use this in case an application needs to support versions of VTK both before and after this change while using vtkDebugLeaks.

I found that most classes that need updating are found by grepping the codebase for the string "new vtk" (assuming all of your vtkObject subclasses start with vtk) and also replacing manual registrations by grepping for "VTK_DEBUG_LEAKS". If the invocation isn't on a vtkCommand/vtkInformationKey subclass, either replace the call with a vtkObjectFactory macro or add a call to newObj->InitializeObjectBase() on the new object.

Dave


On Fri, Sep 9, 2016 at 9:27 AM, David Lonie <[hidden email]> wrote:
On Thu, Sep 8, 2016 at 6:11 PM, David Cole <[hidden email]> wrote:
> A few comments:
>
> That should be typeid(*this), and I'm uncertain if that works in the
> context of a base class constructor before the derived class's
> constructor is called. Does it? Doesn't seem like it should...

typeid will always resolve to the "current" class when called from a
constructor/destructor.

For science: http://codepad.org/WWweG8hk

#include <iostream>

struct Base
{
  Base() { std::cout << typeid(*this).name() << std::endl; }
  virtual ~Base() { std::cout << "~" << typeid(*this).name() << std::endl; }

  void Init() const { std::cout << typeid(*this).name() << std::endl; }
};

struct Derived : public Base
{
};

int main()
{
  Derived d;
  d.Init();
  return 0;
}

Output:
4Base
7Derived
~4Base

The second suggestion in my first email would move the construction
logic to something like the Init() method in that example, which
resolves properly to the Derived class.

> If you do end up tracking every instance, and printing them all at
> leak time, perhaps have a sane way of only printing **some** of them.
> Usually, when I end up with a leak, it's connected to a vast network
> of objects, and they all leak, with hundreds, if not thousands of
> objects leaking.

I agree, digging through the ::Print() output for 100's of objects
would not be useful for most cases! My plan for the object-based
report would be to iterate through the leaked objects, collect
classnames/counts, and print the summary just like it does now by
default. Optionally it could check an environment variable to print a
complete output with object details.

> Also, if you end up tracking every instance, it would be great to have
> an API to the leaks manager to count and iterate all the outstanding
> instances.

That would certainly be useful. Something like void
vtkDebugLeaks::GetCurrentObjects(vtkObjectBaseCollection *col) should
do the trick.

Personally I'm leaning towards the second option (collect strings but
use a centralized vtkObjectBase::InternalInit() method) as it's less
intrusive/less work.

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