Thomas,
On 15 October 2013 16:08:52 Thomas Schatzl <thomas.scha...@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
