JDK-8304743 : Compile_lock and SystemDictionary updates
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 21
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2023-03-22
  • Updated: 2023-04-10
  • Resolved: 2023-04-03
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21
21 b17Fixed
Related Reports
Relates :  
Relates :  
Description
There's code in class loading that takes the Compile_lock when updating the SystemDictionary like:

    { // Grabbing the Compile_lock prevents systemDictionary updates
      // during compilations.
      MutexLocker mu(THREAD, Compile_lock);
      update_dictionary(THREAD, loaded_class, loader_data);
    }

The system dictionary is a concurrent hashtable and is also protected by the SystemDictionary_lock for writes, so shouldn't need the Compile_lock.  The compiler ci code (and jvmci) code takes out the Compile_lock to read the system dictionary to see if the class is loaded, but briefly and doesn't hold the lock while compiling so the class could be loaded before or after the ci code reads it.

I was concerned by this code because when loading a class, we used to have
   {
        MutexLocker ml(Compile_lock);
        add_to_hierarchy();
        update_dictionary();
    }

But JDK-8300926 moved add_to_hierarchy outside of this code block.  add_to_hierarchy needs the Compile_lock because the Compile_lock is also the lock for dependencies, but now it releases the compile lock before updating the dictionary.

Even with the Compile_lock around the update_dictionary() if there's an assert to verify that everything in the hierarchy (subclass, sibling and implementor) is also found in the dictionary, it could fire depending on timing.  But nothing in dependencies does that.

Still the Compile_lock doesn't help.  There used to be a notice_modifications() call that would be a short cut for dependencies that was set during system dictionary updates and read using the Compile_lock.  That code is now gone.

Comments
Changeset: b062b1bd Author: Coleen Phillimore <coleenp@openjdk.org> Date: 2023-04-03 20:25:36 +0000 URL: https://git.openjdk.org/jdk/commit/b062b1bd8126610d9288dc179d69e54a40b81015
03-04-2023

Is there a guarantee that any klasses the compiler found in the hierarchy are actually in the dictionary by the time the compiled code gets to execute? If not, then we could have something like this fail: Class.forName(this.getClass().getName())
31-03-2023

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/13270 Date: 2023-03-31 13:23:31 +0000
31-03-2023

Vladimir Ivanov 15 hours ago CI keeps a cache of previous requests, so if some klass lookup failed before, it won’t try to look it up again. Vladimir Ivanov 14 hours ago CHA relies on siblings list to iterate over subclasses and it will discover the class once add_to_hierarchy is over. It’s covered by the lock. ciEnv::get_klass_by_name_impl() queries SD, but it doesn’t matter whether a concurrent class loading is visible or not. What matters is that compilers consistently see a class as loaded or not. And there’s no need in the lock to have that. Coleen Phillimore 14 hours ago so the compilers save a state that they find when they do the SD lookup in unloaded_klasses and don’t ask again (edited) Vladimir Ivanov 14 hours ago Right. Vladimir Ivanov 14 hours ago IMO it’s safe to remove the lock around update_dictionary call.
31-03-2023

So the adding to unload classes list is a previous behavior, which is unclear is it can cause some problems?
29-03-2023

Another class can't be linked before the deopt for loading has taken effect, because in the loading path, the class isn't in the dictionary until the deopt is done. t1. A. add_to_hierarchy B update_dictionary C t2. won't find class in dictionary until the deopt in add_to_hierarchy done. if lookup at A => adds to the unloaded_klasses list if lookup at B => same if lookup at C => class is loaded and same. The only difference with the Compile_lock around add_to_hierarchy and update_dictionary is that at B, t2 doesn't wait for the deoptimization before adding to the unloaded classes list.
29-03-2023

So I looked a bit at this, it seem like we can be adding an "unloaded class" in CI. It's not clear what effects this have, so I recommend going back to the version of the patch where InstanceKlass::add_to_hierarchy takes the DeoptimizationScope as parameter: ############################### DeoptimizationScope deopt_scope; { MutexLocker mu_r(THREAD, Compile_lock); // Add to class hierarchy, and do possible deoptimizations. add_to_hierarchy(&deopt_scope, k); // Add to systemDictionary - so other classes can see it. // Grabs and releases SystemDictionary_lock update_dictionary(THREAD, k, loader_data); } // Perform the deopt handshake outside Compile_lock. deopt_scope.deoptimize_marked(); ###############################
29-03-2023

Talking about the normal case when we use CHA. The class can be linked before this deopt have taking effect. Meaning another thread can see this loaded class. And that other thread can link this class. But that will trigger a second deopt and that deopt cannot pass the first deopt. And it will not be fully initialized before all deopts are completed. (We are holding the init monitor) So it's not possible to create this class before both deopt have been completed.
28-03-2023

Can other threads see the loaded classes before they're deoptimized? And could that also be a problem?
28-03-2023

I don't understand if Coleen is saying that I introduced a bug or since I did not introduce a bug I proved that we don't need Compile_lock.
23-03-2023

I am saying both. If we actually need the Compile_lock to keep dependencies and dictionary entries consistent, JDK-8300926 introduced a bug. But when I looked at it in the review, I didn't believe that Compile_lock is needed. I am worried about the case: t1: add_to_hierarchy under Compile_lock t2: grab Compile_lock read dependencies -> for some InstanceKlass in hierarchy look it up in Dictionary and not find it t1: take Compile_lock -> add to dictionary We don't look up InstanceKlass in hierarchy in the dependency code as far as I can tell but it would be good to have some way to verify this chain of events is harmless. [~vlivanov]
23-03-2023

> But JDK-8300926 moved add_to_hierarchy outside of this code block. add_to_hierarchy > needs the Compile_lock because the Compile_lock is also the lock for dependencies, but > now it releases the compile lock before updating the dictionary. I raised this in the PR for JDK-8300926 but [~rehn] said it was okay.
22-03-2023