<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Mikael,<br>
<br>
Thanks for the review. I just got back to this. Comments inline,
and new webrev below...<br>
<br>
On 1/28/15 5:03 AM, Mikael Gerdin wrote:<br>
</div>
<blockquote cite="mid:54C8B3F5.4090105@oracle.com" type="cite">Hi
Derek,
<br>
<br>
On 2015-01-27 23:41, Derek White wrote:
<br>
<blockquote type="cite">Review request!
<br>
<br>
This is a change Ramki did before he left that didn't get
checked in.
<br>
The basic idea is to move a bunch fields that encapsulate
collector
<br>
state from G1CollectorPolicy into a separate class "
G1CollectorState".
<br>
<br>
Thanks in advance!
<br>
<br>
- Derek
<br>
<br>
/Bug/: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-7097567">https://bugs.openjdk.java.net/browse/JDK-7097567</a>
<br>
/Webrev/: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/7097567/webrev.00/">http://cr.openjdk.java.net/~drwhite/7097567/webrev.00/</a>
<br>
</blockquote>
<br>
This is just a high level review to begin with, I didn't look too
carefully at all the state changes.
<br>
<br>
I agree that it's a good idea to factor out the state changes but
I'm not convinced that the CollectorState should logically be a
member of the collector policy object.
<br>
To me it feels more like a G1 heap would have a policy object and
a state object as members.
<br>
</blockquote>
I talked this over with Thomas and now understand the issue. I also
sucked up some state fields from
<meta http-equiv="content-type" content="text/html; charset=utf-8">
g1CollectedHeap into g1CollectorState.
<blockquote cite="mid:54C8B3F5.4090105@oracle.com" type="cite">
If the policy object needs to interact with the state object they
can of course do that via a pointer to the state object from the
policy.
<br>
</blockquote>
OK<br>
<blockquote cite="mid:54C8B3F5.4090105@oracle.com" type="cite">
Besides that I don't like the fact that the state object is
declared in the g1CollectorPolicy.hpp header, I'd prefer it if you
could move that to a separate new header file.
<br>
</blockquote>
OK<br>
<blockquote cite="mid:54C8B3F5.4090105@oracle.com" type="cite">
The change to compactibleFreeListSpace seems like an accident
since it has nothing to do with G1 state changes whatsoever. <br>
</blockquote>
OK<br>
<br>
- Derek<br>
<br>
---------- NEW WEBREV ----------<br>
<br>
Bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-7097567">https://bugs.openjdk.java.net/browse/JDK-7097567</a>
<br>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/7097567/webrev.02/">http://cr.openjdk.java.net/~drwhite/7097567/webrev.02/</a><br>
<br>
I also added follow-on enhancement request for turning
G1CollectorState into a real state machine.<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<ul class="breadcrumbs">
<li><a id="key-val" rel="4779486"
href="https://bugs.openjdk.java.net/browse/JDK-8080226">JDK-8080226</a>
G1: Replace collector state booleans with explicit state
variable(s) <br>
</li>
</ul>
<br>
<br>
</body>
</html>