[vtk-developers] memory bloat, with patch

[David Gobbi]

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.  Thanks 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.  You should take
>> a look at what he did and keep your work consistent with his.  See
>> commit 2e68d14f2c59c4163208baf4f935f84034776b6b
>
> I did this, and uploaded a new version:
>
>  http://review.source.kitware.com/1271
>
> but I'm now getting a crash with debug iterators in the
> TestGradientAndVorticity test.  The issue appears to be from
> vtkGradientFilter.cxx, near line 111:
>
>      vtkCell* Cell = Grid->GetCell(Index);
>      double pcoords[3];
>      int subId = Cell->GetParametricCenter(pcoords);
>      vtkstd::vector<double> weights(Cell->GetNumberOfPoints());
>      Cell->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.  I 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:
>> >
>> > =A0http://review.source.kitware.com/1259
>> >
>> > which fortunately still applied to current master. =A0Builds fine on
>> > Linux, and some spot tests indicate nothing is amiss... plus the diff
>> > itself is pretty straightforward. =A0This cuts down on our memory use in
>> > VisIt pretty significantly.
>> >
>> > Please review || apply.
>> >
>> > -tom
>