<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 John,<br>
<br>
This looks good to me. Just like I thought ;)<br>
<br>
One nit if you feel like fixing it before you push:<br>
<br>
In a couple places you have this pattern:<br>
<br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
2240 _task->do_marking_step(mark_step_duration_ms,<br>
2241 false /* do_termination
*/,<br>
2242 _is_serial /* is_serial
*/);<br>
<br>
I think the comment /* is_serial */ is superfluous now that we
pass a variable named _is_serial instead of just true or false.<br>
<br>
Bengt<br>
<br>
<br>
On 3/14/13 6:19 PM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:51420692.10802@oracle.com" type="cite">Hi
Everyone,
<br>
<br>
Here's the final webrev based upon Bengt's comments:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~johnc/8009536/webrev.2/">http://cr.openjdk.java.net/~johnc/8009536/webrev.2/</a>
<br>
<br>
I think I'm all set after reviews from Bengt and Thomas - I just
wanted to send out the final version of the webrev in case anyone
else has other comments.
<br>
<br>
Thanks,
<br>
<br>
JohnC
<br>
<br>
On 3/13/2013 2:01 PM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi John,
<br>
<br>
Thanks for explaining a bit about the serial case where we still
need to go through termination protocol.
<br>
<br>
As you know I'll be on vacation the next two days, so I just
wanted to let you know that with the changes that you outline
below (the fix to CMRemarkTask and removing the do_stealing
argument) I'm fine with this change. Feel free to push this if
you get another review.
<br>
<br>
Thanks,
<br>
Bengt
<br>
<br>
On 3/13/13 6:57 PM, John Cuthbertson wrote:
<br>
<blockquote type="cite">Hi Bengt,
<br>
<br>
Thanks for looking at the change again. Replies inline....
<br>
<br>
On 3/13/2013 3:22 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
<br>
Hi John,
<br>
<br>
I'm not too familiar with the do_marking_step() code, so I'm
trying to get to know it while I'm looking at your webrev.
So, maybe I'm completely off here, but I think there are a
couple of strange things.
<br>
<br>
After your change do_marking_step() takes three bools:
do_stealing, do_termination and is_serial.
<br>
<br>
This means that we have 8 possible states if we combine all
value of these bools. I went through all calls to
do_marking_step() and it seems like we make use of 4 of
those states:
<br>
<br>
<br>
G1CMKeepAliveAndDrainClosure::do_oop_work()
<br>
do_marking_step(false, false, true) (1)
<br>
<br>
CMConcurrentMarkingTask::work()
<br>
do_marking_step(true, true, false) (2)
<br>
<br>
CMRemarkTask::work()
<br>
do_marking_step(true, true, false) (2)
<br>
do_marking_step(true, true, true) (3)
<br>
<br>
G1CMDrainMarkingStackClosure::do_void()
<br>
do_marking_step(false, true, true) (4)
<br>
do_marking_step(true, true, false) (2)
<br>
<br>
</blockquote>
<br>
Yeah. Well...
<br>
<br>
<blockquote type="cite">
<br>
I numbered the states (1) - (4). Here are all states and I
marked which ones we use:
<br>
<br>
stealing, termination, serial (3)
<br>
stealing, !termination, serial
<br>
stealing, !termination, !serial
<br>
stealing, termination, !serial (2) parallel
<br>
!stealing, termination, serial (4)
<br>
!stealing, !termination, serial (1) serial
<br>
!stealing, !termination, !serial
<br>
!stealing, termination, !serial
<br>
<br>
<br>
I think state (2) makes sense as the parallel case. We do
stealing and termination and we are not serial. State (1)
also makes sense. It is clearly the serial case. No stealing
or termination. Just serial.
<br>
<br>
Now, case (3) looks odd to me. We say we are serial but we
want to do stealing and termination. This case comes from
CMRemarkTask. It takes a is_serial argument in its
constructor. We get to case (3) from
ConcurrentMark::checkpointRootsFinalWork(), where we do:
<br>
<br>
if (G1CollectedHeap::use_parallel_gc_threads()) {
<br>
...
<br>
} else {
<br>
...
<br>
CMRemarkTask remarkTask(this, active_workers, true /*
is_serial*/);
<br>
}
<br>
<br>
To me this looks like it should lead to the pure serial case
(1) instead of (3). Do you agree? In that case I think
CMRemarkTask::work() needs to be changed to do:
<br>
<br>
task->do_marking_step(1000000000.0 /* something
very large */,
<br>
!_is_serial /* do_stealing */,
<br>
!_is_serial /* do_termination
*/,
<br>
_is_serial);
<br>
<br>
</blockquote>
<br>
You're right here. The serial remark case should not enable
stealing. It doesn't make sense. Done!
<br>
<br>
<blockquote type="cite">
<br>
The other case that sticks out is (4). We don't want to do
stealing, but we want to do termination even though we are
serial. This comes from how we set up the
g1_drain_mark_stack in ConcurrentMark::weakRefsWork(). We
pass that along to the reference processing, which I think
is serial (wasn't that the original problem? :) ). We also
pass the g1_keep_alive which is set up to be in state (1).
So, I wonder if it is correct that the g1_drain_mark_stack
is in state (4). If you agree that we should be using the
pure serial case here too, I think we need to change
G1CMDrainMarkingStackClosure::do_void() to be:
<br>
<br>
_task->do_marking_step(1000000000.0 /* something
very large */,
<br>
!_is_serial /* do_stealing
*/,
<br>
!_is_serial /* do_termination
*/,
<br>
_is_serial /* is_serial */);
<br>
<br>
<br>
</blockquote>
<br>
That is what we had before and I wasn't happy with it. The
serial drain closure is the last thing that get's executed as
part of reference processing - when process_phaseJNI invokes
the complete_gc.do_void() method. In the drain closure either
serial or parallel I think we want to go through the
termination protocol. We just don't want to interact with the
terminator in the serial case. There's a bunch of guarantees
and checks in the termination protocol and we should still go
through them - even when serial.
<br>
<br>
<blockquote type="cite">
<br>
Again, I'm not sure about this, but if the above is correct
we will be left with only two states. The parallel and the
serial. In that case I think it would make sense to remove
the do_stealing and do_termination arguments for
do_marking_step() and just pass in the is_serial argument.
Internally do_marking_step() could set up two local variable
to track stealing and termination. That might be good if we
see the need in the future to split these up again.
<br>
</blockquote>
<br>
I think we can get rid of the do_stealing parameter - it's
always the negation of the is_serial parameter and I like the
idea of making it a local. How about that? For example
<br>
<br>
void CMTask::do_marking_step(double ms, bool do_termination,
bool is_serial) {
<br>
// It does not make sense to allow stealing when called
serially.
<br>
bool do_stealing = !is_serial;
<br>
<br>
Thanks,
<br>
<br>
JohnC
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>