<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Thomas, <br>
<br>
Thanks for looking at this webrev!<br>
<br>
On 2/12/14 7:10 PM, Thomas Schatzl wrote:<br>
</div>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
Hi,
On Tue, 2014-02-11 at 11:50 +0100, Bengt Rutisson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi again,
I fixed the serviceability agent (SA) and couple of minor things (in
particular HeapRegionSetBase::_is_linked was not used). Here is an
updated webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8034079/webrev.01/">http://cr.openjdk.java.net/~brutisso/8034079/webrev.01/</a>
Here is the diff compared to the previous version in case anybody has
started looking at it:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8034079/webrev00.01.diff/">http://cr.openjdk.java.net/~brutisso/8034079/webrev00.01.diff/</a>
Thanks,
Bengt
On 2014-02-10 14:04, Bengt Rutisson wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
Hi everyone,
Could I have a couple of reviews for this change?
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8034079/webrev.00/">http://cr.openjdk.java.net/~brutisso/8034079/webrev.00/</a>
It tries to simplify the structure of the HeapRegionSets. I need this
to be able to introduce new types of heap region collections.
Some other cleanups are done as a consequence of this. But there are
probably still more cleanups to do. I hope that this is a step in the
right direction.
Here's a short list of some of the changes:
- Class hierarchy simplified
No longer specific subclasses for all instances. Verification code has
been collected in the super classes and the MT safety checks are
aggregated instead.
- Proxy sets replaced by HeapRegionSetCount
The use of "proxy sets" was removed in favor of a light weight class
to keep track of length and capacity.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
- is it required to keep track of both length and capacity? To me it
seems that the value passed to capacity is strictly always length *
HeapRegionSize.</pre>
</blockquote>
<br>
We need both due to the kind of strange way we handle humongous
regions. We only add the humongous start regions, but their capacity
is the capacity of the start humongous region and all the
corresponding continues humongous regions. It would be nice to
implement that differently in my opinion, but it is out of scope for
this change.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- G1CollectedHeap::free_region_if_empty() was inlined
</pre>
</blockquote>
</blockquote>
<pre wrap="">
Seems okay. At least I saw no better way immediately. I will think about it.</pre>
</blockquote>
<br>
Good.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- G1CollectedHeap::update_sets_after_freeing_regions() was split up
into three (or actually four) methods.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
Seems a good idea because very frequently it got passed many constant NULL or 0
values for parameters anyway.</pre>
</blockquote>
<br>
Yes, that's what I thought.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">
- Removed the HRSPhaseSetter, which may soften the verification a bit
but not much.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
I do not mind. Does not seem really important.</pre>
</blockquote>
<br>
Agreed.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- The verification code also prints less information in some cases.
But instead it prints the relevant information. There is still more
cleanup that can be done to the messages from the asserts and
verifications.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
:)
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">- Moved usage accounting up to avoid having to pass it around as much.
- HeapRegionSetBase::total_used_bytes() was only used in asserts. Did
not seem worth keeping around.
After these changes the file heapRegionSets.hpp (notice the s at the
end of the name) was empty so I removed it. However, the
heapRegionSets.cpp file is still present. I would like to move all the
code there in to heapRegionSet.cpp, but that would make the webrev
more difficult to follow. I'm still open to doing that if someone
would like me to do it.
</pre>
</blockquote>
</blockquote>
<pre wrap="">
I am all for it, bug a separate CR would be fine for me.</pre>
</blockquote>
<br>
Great. I'll file a CR for that.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
- In heapRegionSet.hpp there is a comment about a "HeapRegionLinkedList" which does not exist any more.</pre>
</blockquote>
<br>
Thanks for catching that. I actually went with your suggestion below
to just remove all ///////// comments.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
- in FreeRegionList::remove_head_or_null() the MT check seems missing now.</pre>
</blockquote>
<br>
Right. Added it back. Good catch!<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
- heapRegionSets.cpp contains a "//// MasterFreeRegionList ///" comment that seems obsolete. Maybe all these lines with the many slashes could be removed or changed to something less obtrusive ;)</pre>
</blockquote>
<br>
Yes, I removed all the ///////// comments. I agree that they are
mostly in the way.<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
- in heapRegionSet.cpp some verification code is duplicated (e.g. line 158). Could this be factored out? (around the comment "// In set_containing_set() we check that we either...").</pre>
</blockquote>
<br>
Good point. I added a common method called
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
add_as_head_or_tail().<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
Overall I like this refactoring :)</pre>
</blockquote>
<br>
Thanks!<br>
<br>
Updated webrev here:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8034079/webrev.02/">http://cr.openjdk.java.net/~brutisso/8034079/webrev.02/</a><br>
<br>
And here is the diff compared to the 01 version:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8034079/webrev.01-02.diff/">http://cr.openjdk.java.net/~brutisso/8034079/webrev.01-02.diff/</a><br>
<br>
Thanks again for looking at this!<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:1392228623.2724.2.camel@cirrus" type="cite">
<pre wrap="">
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>