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). I do not mind either way. Thanks, Thomas
