<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
On 2/23/23 9:54 PM, David Holmes wrote:<br>
<blockquote type="cite" cite="mid:37uJ_WR0M5Q9Te1lWg2ecfCTGgFzzecrAPmmlnQ8vzQ=.f756a746-641e-4b29-a336-aeb549a74ff4@github.com">
<pre class="moz-quote-pre" wrap="">On Thu, 23 Feb 2023 12:46:47 GMT, Robbin Ehn <a class="moz-txt-link-rfc2396E" href="mailto:rehn@openjdk.org"><rehn@openjdk.org></a> wrote:
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">I found one call to `add_to_hierarchy()` where we previously did not
grab in `Compile_lock` in `vmClasses::resolve_shared_class()` and
that call is made with this assert in place:
```assert(!Universe::is_fully_initialized(), "We can make short cuts only during VM initialization");```
so it looks to me like we have a locking issue there (as in too early for locks).
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I think you are taking about CDS dump process. It works fine taking the lock.
I think this was just some micro-optimization. But we should not mark and do deopt fixed that.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
To restate: the assert_locked_or_safepoint is pointless because you just took out the lock here.
The code Dan points to does not seem to be related to CDS dump:
bool vmClasses::resolve(vmClassID id, TRAPS) {
InstanceKlass** klassp = &_klasses[as_int(id)];
#if INCLUDE_CDS
if (UseSharedSpaces && !JvmtiExport::should_post_class_prepare()) {
InstanceKlass* k = *klassp;
assert(k->is_shared_boot_class(), "must be");
ClassLoaderData* loader_data = ClassLoaderData::the_null_class_loader_data();
resolve_shared_class(k, loader_data, Handle(), CHECK_false);
return true;
}
#endif // INCLUDE_CDS
I remain concerned this code may be executed at a safepoint and we will now try to take the Compile_lock.
-------------
PR: <a class="moz-txt-link-freetext" href="https://git.openjdk.org/jdk/pull/12585">https://git.openjdk.org/jdk/pull/12585</a>
</pre>
</blockquote>
<font face="monospace"> <br>
For some reason these comments no longer appear in the current PR.<br>
That's probably because the code has changed relative to the
original<br>
anchors for the comments so the bots don't know where in the code
to<br>
attach the comments.<br>
<br>
In any case, here's the current code that we were talking about
(from<br>
the v05 full webrev):<br>
<br>
</font><br>
<pre><span class="udiff-line-modified-removed">-void SystemDictionary::add_to_hierarchy(InstanceKlass* k) {
</span><span class="udiff-line-modified-added">+void SystemDictionary::add_to_hierarchy(JavaThread* current, InstanceKlass* k) {
</span><span> assert(k != nullptr, "just checking");
</span><span class="udiff-line-modified-removed">- if (Universe::is_fully_initialized()) {
</span><span class="udiff-line-modified-removed">- assert_locked_or_safepoint(Compile_lock);
</span><span class="udiff-line-modified-removed">- }
</span><span class="udiff-line-modified-added">+ assert(!SafepointSynchronize::is_at_safepoint(), "must NOT be at safepoint");
</span><span class="udiff-line-modified-added">+ DeoptimizationScope deopt_scope;
</span><span class="udiff-line-modified-added">+ {
</span><span class="udiff-line-modified-added">+ MutexLocker ml(current, Compile_lock);
So the "assert_locked_or_safepoint(Compile_lock)" call has been replaced by an
unconditional:
assert(!SafepointSynchronize::is_at_safepoint(), "must NOT be at safepoint");
which should trip if SystemDictionary::add_to_hierarchy() is ever called
from a safepoint (with non-release bits). As far as I know, this assert has
not fired in Robbin's testing.
Also, the code that I was talking about was not "bool vmClasses::resolve(vmClassID id, TRAPS)".
I was talking about "vmClasses::resolve_shared_class()" which is the code that
calls this assert:
assert(!Universe::is_fully_initialized(), "We can make short cuts only during VM initialization");
I was worried about this code path because its the only place that calls
add_to_hierarchy() in the original code that _did not_ grab Compile_lock.
With Robbin's latest code add_to_hierarchy() does grab Compile_lock, but
if that was going to be a problem, it should have shown up in testing.
Dan
</span></pre>
<font face="monospace"></font>
</body>
</html>