[vtk-developers] memory bloat, with patch

[tom fogal]

Hi, sorry for letting this drop, I went on travel for ~3 weeks.

Anyway I tried this and still couldn't get the test to pass.  I did
narrow it down to the vtkStructuredGrid changes though; with just the
rectilinear grid modifications the test passes fine.

I am currently attempting to run more extensive tests on just the RGrid
part, and then I'll begin to slowly fiddle + very carefully redo the
structured grid changes until I can figure out which hunk is causing
the problem.

Thanks for the help,

-tom

David Gobbi <david.gobbi at gmail.com> writes:
> Hi Tom,
> 
> I think that I've found the problem.  It is in the SetExtent() method of th=
> e
> modified classes.
> 
> The call to this->SetDataDescription() has to occur before the check for
> if (description =3D=3D 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=3D0 check was
> > the main issue.
> >
> > It looks like line 544 has a similar issue:
> > cell->Derivatives(subId, parametricCoord, &values[0], 1, derivative);
> >
> > =A0- 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. =A0I 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. =A0This is the 'GradCellArray' in the test code.
> >>
> >> 1272 this time:
> >>
> >> =A0http://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. =A0If you fixed
> >>> vtkGradientFilter, I think that would be the best solution.
> >>>
> >>> A quick note about GetDataDescription: note that in vtkImageData.h it i=
> s
> >>> inlined like so:
> >>>
> >>> =A0 int GetDataDescription() { return this->DataDescription; }
> >>>
> >>> There is a reason for this. =A0The vtkGetMacro creates a virtual method=
> ,
> >>> and virtual methods are not inlined. =A0Considering how liberally these=
>  data
> >>> classes use GetDataDescription() internally, we definitely want to avoi=
> d
> >>> the virtual function call overhead.
> >>>
> >>> =A0- 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. =3DA0Thanks 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. =3DA0You should =
> take
> >>> >> a look at what he did and keep your work consistent with his. =3DA0S=
> ee
> >>> >> commit 2e68d14f2c59c4163208baf4f935f84034776b6b
> >>> >
> >>> > I did this, and uploaded a new version:
> >>> >
> >>> > =3DA0http://review.source.kitware.com/1271
> >>> >
> >>> > but I'm now getting a crash with debug iterators in the
> >>> > TestGradientAndVorticity test. =3DA0The issue appears to be from
> >>> > vtkGradientFilter.cxx, near line 111:
> >>> >
> >>> > =3DA0 =3DA0 =3DA0vtkCell* Cell =3D3D Grid->GetCell(Index);
> >>> > =3DA0 =3DA0 =3DA0double pcoords[3];
> >>> > =3DA0 =3DA0 =3DA0int subId =3D3D Cell->GetParametricCenter(pcoords);
> >>> > =3DA0 =3DA0 =3DA0vtkstd::vector<double> weights(Cell->GetNumberOfPoin=
> ts());
> >>> > =3DA0 =3DA0 =3DA0Cell->EvaluateLocation(subId, pcoords, Coords, &weig=
> hts[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 differe=
> nt
> >>> > type in that case. =3DA0I 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> wro=
> te:
> >>> >> > One of the devs I work with sent along this patch:
> >>> >> >
> >>> >> > =3D3DA0http://review.source.kitware.com/1259
> >>> >> >
> >>> >> > which fortunately still applied to current master. =3D3DA0Builds f=
> ine on
> >>> >> > Linux, and some spot tests indicate nothing is amiss... plus the d=
> iff
> >>> >> > itself is pretty straightforward. =3D3DA0This cuts down on our mem=
> ory us=3D
> >>> e in
> >>> >> > VisIt pretty significantly.
> >>> >> >
> >>> >> > Please review || apply.
> >>> >> >
> >>> >> > -tom