<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><br>
Hi Stefan,<br>
<br>
Thanks for looking at this!<br>
<br>
On 6/11/14 1:10 PM, Stefan Karlsson wrote:<br>
</div>
<blockquote cite="mid:53983933.2020204@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 2014-06-11 12:33, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:5398305D.7070800@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<br>
Hi all,<br>
<br>
Can I have a review for this change?<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebrutisso/8046518/webrev.00/">http://cr.openjdk.java.net/~brutisso/8046518/webrev.00/</a><br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8046518">https://bugs.openjdk.java.net/browse/JDK-8046518</a><br>
<br>
Background:<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
When we abort a concurrent cycle due to a Full GC in G1 we call
ConcurrentMark::abort(). That will set _has_aborted flag and
then call register_concurrent_cycle_end(). <br>
<br>
The concurrent marking thread will see the _has_aborted flag in
its ConcurrentMarkThread::run() method, abort the execution and
then call register_concurrent_cycle_end(). <br>
<br>
Currently this works since the code inside
register_concurrent_cycle_end() is guarded by
_concurrent_cycle_started which it then resets. So, the double
calls will not necessarily result in too much extra work being
done. But one of the things that register_concurrent_cycle_end()
does is to call report_gc_end() on the concurrent GC tracer.
That prevents further use of it for this GC. This means that
inside the ConcurrentMarkThread::run() method we can not rely on
the tracer. <br>
<br>
Removing the call to register_concurrent_cycle_end() in
ConcurrentMark::abort() and relying on the call in
ConcurrentMarkThread::run() seems to be a reasonable approach. <br>
</blockquote>
<br>
The double call was deliberately put there to make sure that we
end the tracing of the concurrent GC before starting to trace teh
Full GC. </blockquote>
<br>
I figured there was a reason. I just couldn't remember. We would get
overlapping GC events without this extra call. Thanks for pointing
that out!<br>
<br>
<blockquote cite="mid:53983933.2020204@oracle.com" type="cite">Why
do you need to change this? I guess it has to do with your other
GCId changes?<br>
</blockquote>
<br>
Right. It is for the GCId change. The problem is that calling
register_concurrent_cycle_end() will reset the GCId to be -1. When
we get to the logging, which is done in ConcurrentMarkThread::run(),
I want to add the GCId to this log entry:<br>
<br>
if (cm()->has_aborted()) {<br>
if (G1Log::fine()) {<br>
gclog_or_tty->gclog_stamp(g1h->gc_tracer_cm()->gc_id());<br>
gclog_or_tty->print_cr("[GC concurrent-mark-abort]");<br>
}<br>
}<br>
<br>
But with the current code the GCId is always -1 here.<br>
<br>
I guess one workaround I can do is to in abort() store the last
aborted GC id and use that for logging. It just seems a bit fragile
that we reset the concurrent gc tracer while we still have the
concurrent mark running.<br>
<br>
Bengt<br>
<br>
<br>
<blockquote cite="mid:53983933.2020204@oracle.com" type="cite"> <br>
thanks,<br>
StefanK<br>
<br>
<blockquote cite="mid:5398305D.7070800@oracle.com" type="cite"> <br>
Thanks,<br>
Bengt<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>