[vtk-developers] memory bloat, with patch

[tom fogal]

Thanks.

The vector would prevent a leak in the case of an exception, but if
that's not a concern... anyway, done.  I hadn't realized / at least
forgot the macro made virtual functions, fixed that too.

Anyway, I had tried checking for 0 points before the last revision as
well: it fixes the crash, but the test still complains about an invalid
result.  This is the 'GradCellArray' in the test code.

1272 this time:

  http://review.source.kitware.com/1272

-tom

David Gobbi <david.gobbi at gmail.com> writes:
> That vtkGradientFilter.cxx code looks buggy to me, it should be
> doing a check to make sure that there are more than zero points.
> And it should be using new/delete instead of using an stl vector
> and then grabbing a pointer to the vector internals.  If you fixed
> vtkGradientFilter, I think that would be the best solution.
> 
> A quick note about GetDataDescription: note that in vtkImageData.h it is
> inlined like so:
> 
>   int GetDataDescription() { return this->DataDescription; }
> 
> There is a reason for this.  The vtkGetMacro creates a virtual method,
> and virtual methods are not inlined.  Considering how liberally these data
> classes use GetDataDescription() internally, we definitely want to avoid
> the virtual function call overhead.
> 
>  - David
> 
> 
> On Mon, Mar 28, 2011 at 11:56 AM, tom fogal <tfogal at sci.utah.edu> wrote:
> > David Gobbi <david.gobbi at gmail.com> writes:
> >> It's a good change, but I found one mistake (noted in the review) so
> >> it needs re-checking.
> >
> > Okay, fixed. =A0Thanks for the review.
> >
> >> I did a "git blame" on vtkImageData.cxx, and it turns out that Berk
> >> did a very similar fix there just a few months ago. =A0You should take
> >> a look at what he did and keep your work consistent with his. =A0See
> >> commit 2e68d14f2c59c4163208baf4f935f84034776b6b
> >
> > I did this, and uploaded a new version:
> >
> > =A0http://review.source.kitware.com/1271
> >
> > but I'm now getting a crash with debug iterators in the
> > TestGradientAndVorticity test. =A0The issue appears to be from
> > vtkGradientFilter.cxx, near line 111:
> >
> > =A0 =A0 =A0vtkCell* Cell =3D Grid->GetCell(Index);
> > =A0 =A0 =A0double pcoords[3];
> > =A0 =A0 =A0int subId =3D Cell->GetParametricCenter(pcoords);
> > =A0 =A0 =A0vtkstd::vector<double> weights(Cell->GetNumberOfPoints());
> > =A0 =A0 =A0Cell->EvaluateLocation(subId, pcoords, Coords, &weights[0]);
> >
> > The 'Cell' that gets returned here is a vtkEmptyCell, the number of
> > points is 0, and thus the vector is of size 0 and dereferencing it is
> > illegal.
> >
> > It's just fine with master, so I guess it must be returning a different
> > type in that case. =A0I can't quite see what's gone wrong... could you
> > (or anyone) take a look?
> >
> > -tom
> >
> >> On Fri, Mar 25, 2011 at 4:14 PM, tom fogal <tfogal at sci.utah.edu> wrote:
> >> > One of the devs I work with sent along this patch:
> >> >
> >> > =3DA0http://review.source.kitware.com/1259
> >> >
> >> > which fortunately still applied to current master. =3DA0Builds fine on
> >> > Linux, and some spot tests indicate nothing is amiss... plus the diff
> >> > itself is pretty straightforward. =3DA0This cuts down on our memory us=
> e in
> >> > VisIt pretty significantly.
> >> >
> >> > Please review || apply.
> >> >
> >> > -tom
> >