RFR (S): 8025925: jmap fails with "field _length not found in type HeapRegionSeq"

[Mikael Gerdin]

Thomas,

On 15 October 2013 16:08:52 Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> Hi,
>
> On Tue, 2013-10-15 at 15:04 +0200, Mikael Gerdin wrote:
> > Thomas,
> > On Tuesday 15 October 2013 12.53.25 Thomas Schatzl wrote:
> > > Hi all,
> > > >   can I have reviews for the following change? It updates the SA agent
> > > that got out of date after the changes for JDK-7163191 where the
> > > HeapRegionSeq class has been refactored.
> > > > The changes mirror the changes in the C++ code of that revision, adding
> > > a G1HeapRegionTable java class, and the associated modifications in the
> > > HeapRegionSeq class.
> > > > There have been no interface changes to the HeapRegionSeq class to avoid
> > > breakage of any tools depending on it.
> > > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8025925/webrev/
> > Why did you change the copyright header format?
> > "2011, 2013" is the format we should use, where 2011 is the first year 
> and 2013 is the year when it was last modified.
>
> Okay, sorry. Fixed. Not sure what I thought when changing that.
>
> > Otherwise I think the changes look good. Do you know if any part of the 
> SA code base actually uses this class?
>
> Which one? HeapRegionSeq or the new G1HeapRegionTable class?
> HeapRegionSeq is used by the G1CollectedHeap class for the n_regions()
> and heap iteration during heap dump and liveness analysis it seems. In
> the change, the G1HeapRegionTable replaced the _regions field of
> HeapRegionSeq. I tried to follow what I thought was the style of the
> other code, i.e. try to stay close to the C++ object hierarchy.
>
> The new HeapRegionSeq mostly forwards requests to it to the new
> G1HeapRegionTable class. I could hide the latter (i.e. make it package
> private) if wanted and even move it into the HeapRegionSeq.java file
> (and remove the new file).

Ok, in that case I'm fine with the code change as-is.
I don't need to see the copyright year fix.

Ship it,
/Mikael

>
> I do not mind either way.
>
> Thanks,
> Thomas
>
>