<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>
Your changes look good to me.<br>
<br>
I think your motivation for removing the verification from
universe2_init() and init_globals() is fine. Actually I wonder why
they were there in the first place, but they do seem intentionally
put in there. However, I'm fine with removing them.<br>
<br>
About the test. Great that you write a regression test for this!
:)<br>
<br>
The @summary says that the test uses
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
-XX:+VerifyDuringGC but the command line is actually using
-XX:+VerifyBeforeGC (which is correct, I think). Also, would it
make sense to have a separate test that specifies -XX:+UseG1GC and
checks the output that we expect to see?<br>
<br>
One question that is not strictly related to your change:<br>
<br>
The code to do the verification in Threads::create_vm() is:<br>
<br>
3449 if (VerifyBeforeGC &&<br>
3450 Universe::heap()->total_collections() >=
VerifyGCStartAt) {<br>
3451 Universe::heap()->prepare_for_verify();<br>
3452 Universe::verify(); // make sure we're starting with a
clean slate<br>
3453 }<br>
<br>
This is what it looked like before your change as well. But to me
this looks kind of odd. First, we re-use the flag VerifyBeforeGC
even though we are not about to do a GC. I can live with that, but
it is kind of strange. Then we have the check against
VerifyGCStartAt. By default this is 0 so we will do the
verification. But why do we do this check? There is no chance that
we have been able to do any GC yet, right? So, checking against
Universe::heap()->total_collections() seems unnecessary. We
should either check VerifyGCStartAt == 0 or not include that flag
at all (best choice in my opinion).<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<br>
On 3/21/13 11:28 PM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:514B8996.8030909@oracle.com" type="cite">Hi
Everyone,
<br>
<br>
I'm looking for a couple of reviews for the fix for these crashes.
The webrev can be found at:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~johnc/8010463/webrev.0/">http://cr.openjdk.java.net/~johnc/8010463/webrev.0/</a>
<br>
<br>
Summary:
<br>
During JVM start up, with TLABs disabled, the JVM performs three
separate verifications. The first is in universe2_init(), the
second is in init_globals(), and the final one is in
Threads::create_vm(). With TLABs enabled only one verification is
performed during start up - the one in Threads::create_vm().
These verifications are invoked by the main thread.
<br>
<br>
The problem here was that the G1 verification code was expecting
to be invoked by the VMThread, at a safepoint. When TLABs are
disabled the verification code was executed by main thread,
triggering the assert. Relaxing the assert (to allow for execution
during VM start up) is, unfortunately, not a good solution. There
are parts of the root scanning code which assume the JVM is at a
safepoint or has completed initialization. For example
Threads::oops_do() assumes that the VMThread exists;
CodeCache::oops_do() assumes that the CodeCache_lock is being held
(or the JVM is at a safepoint); verifying G1's region sets assumes
that the Heap_lock is being held (or the JVM is at a safepoint);
etc.
<br>
<br>
When TLABs are enabled the verification from Threads::create_vm()
skips verifying parts of the heap. The solution is to skip those
parts of the verification even if TLABs are disabled. With just
the changes in g1CollectedHeap.cpp, we would see the following:
<br>
<br>
With -UseTLABS:
<br>
<br>
<blockquote type="cite">----> universe2_init
<br>
[Verifying threads (SKIPPING roots, heapRegionSets, heapRegions,
remset) syms strs zone dict cldg metaspace chunks hand C-heap
code cache ]
<br>
<---- universe2_init
<br>
---->init::verify
<br>
[Verifying threads (SKIPPING roots, heapRegionSets, heapRegions,
remset) syms strs zone dict cldg metaspace chunks hand C-heap
code cache ]
<br>
<----init::verify
<br>
--->create_vm:verify
<br>
[Verifying threads (SKIPPING roots, heapRegionSets, heapRegions,
remset) syms strs zone dict cldg metaspace chunks hand C-heap
code cache ]
<br>
<---create_vm:verify
<br>
</blockquote>
<br>
With +UseTLABS:
<br>
<br>
<blockquote type="cite">----> universe2_init
<br>
<---- universe2_init
<br>
---->init::verify
<br>
<----init::verify
<br>
--->create_vm:verify
<br>
[Verifying threads (SKIPPING roots, heapRegionSets, heapRegions,
remset) syms strs zone dict cldg metaspace chunks hand C-heap
code cache ]
<br>
<---create_vm:verify
<br>
</blockquote>
<br>
Why do we perform two additional verifications when TLABs are
disabled? I've removed these in this fix. If someone can provide a
reasonable justification, I'll add them back.
<br>
<br>
Additionally I've moved the verification code in
Threads::create_vm() to after the VMThread is created. That way,
as a future enhancement, the verification could be wrapped inside
a VMOperation.
<br>
<br>
I've also included a regression test.
<br>
<br>
Testing:
<br>
The failing test case with G1 with and without TLABs enabled.
<br>
The regression test with all the collectors.
<br>
A jprt run (with -UseTLABS -XX:+VerifyBeforeGC) is in the queue.
<br>
<br>
Thanks,
<br>
<br>
JohnC
<br>
<br>
<br>
<br>
</blockquote>
<br>
</body>
</html>