<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<div class="moz-cite-prefix">On 3/27/2014 8:18 AM, Mikael Gerdin
wrote:<br>
</div>
<blockquote cite="mid:2144934.Jed7xMUM7G@mgerdin03" type="cite">
<pre wrap="">Hi,
On Wednesday 26 March 2014 17.22.03 Mikael Gerdin wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi,
I'm doing some cleanups in the "Space" classes to simplify the code for the
G1 class unloading work[1].
This change moves around some virtual functions in Space to more (in my
mind) sane places:
* Space::is_in is implemented the same by all Space subclasses, move it to
Space and make it non-virtual.
* Space::used_region has a bogus implementation that is never useful, remove
it.</pre>
</blockquote>
</blockquote>
<br>
Minor point but used_region() did do as the comment says. It just<br>
most often returned a region that include more than just the
allocated<br>
objects, and not a proper subregion. Could you change the comment
to<br>
<br>
<pre> 136 // Returns a subregion of the space containing <font color="#ff0000">only </font>the <font color="#ff0000">allocated </font>objects in
137 // the space.</pre>
if that is how it is used in all the other spaces. <br>
<br>
Rest looks good.<br>
<br>
Reviewed.<br>
<br>
Jon<br>
<br>
<blockquote cite="mid:2144934.Jed7xMUM7G@mgerdin03" type="cite">
<blockquote type="cite">
<pre wrap="">
* Consolidate more code related to saved_marks in the Space base class
</pre>
</blockquote>
<pre wrap="">
When making set_saved_mark non-virtual G1OffsetTableContigSpace's override of
set_saved_mark became a shadowing function instead. The code still works fine
since all calls to G1OffsetTableContigSpace::set_saved_mark are on pointers to
HeapRegion and not the base class.
To clarify that G1 uses the "saved_mark" functionality for a completely
different purpose I have renamed the G1 function to record_top_and_timestamp:
Incremental webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0.to.1">http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0.to.1</a>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgerdin/8038405/webrev.1">http://cr.openjdk.java.net/~mgerdin/8038405/webrev.1</a>
/Mikael
</pre>
<blockquote type="cite">
<pre wrap="">* Make CompactibleSpace::reset_after_compaction a pure virtual.
* Give Space::minimum_free_block_size a default implementation since only
CMS needs to override it.
Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8038405">https://bugs.openjdk.java.net/browse/JDK-8038405</a>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0">http://cr.openjdk.java.net/~mgerdin/8038405/webrev.0</a>
/Mikael
[1] <a class="moz-txt-link-freetext" href="http://openjdk.java.net/jeps/156">http://openjdk.java.net/jeps/156</a>
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>