[vtk-developers] memory bloat, with patch

David Gobbi david.gobbi at gmail.com
Mon Mar 28 16:38:58 EDT 2011


Hi Tom,

I think that I've found the problem.  It is in the SetExtent() method of the
modified classes.

The call to this->SetDataDescription() has to occur before the check for
if (description == VTK_UNCHANGED), or else in some cases it will not
be called at all.

This was not a problem in vtkImageData.cxx because it initializes its
extent to an invalid extent {0, -1, 0, -1, 0, -1} but I'm not sure if that would
be appropriate for these two classes.

As for vtkGradientFilter, I recommend changing it back to the way it was
originally.  I took a deeper look, and it would need many changes in order
to give correct results for data sets with empty cells.  Best to leave it
as-is.

 - David


On Mon, Mar 28, 2011 at 1:54 PM, David Gobbi <david.gobbi at gmail.com> wrote:
> Don't worry about the vector so much... My complaint about it has more
> to do with my style preferences than anything, the size=0 check was
> the main issue.
>
> It looks like line 544 has a similar issue:
> cell->Derivatives(subId, parametricCoord, &values[0], 1, derivative);
>
>  - David
>
>
>
> On Mon, Mar 28, 2011 at 1:35 PM, tom fogal <tfogal at sci.utah.edu> wrote:
>> 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
>>> >
>>
>



More information about the vtk-developers mailing list