Investigate interpreter mirror roots need/reliability

XMLWordPrintable

    • Type: Bug
    • Resolution: Unresolved
    • Priority: P4
    • 27
    • Affects Version/s: None
    • Component/s: hotspot

      Noticed this while looking at related code. JDK-8154580 started to store the klass mirror in interpreter frames, so that we do not lose the GC paths to the klasses:

      ```
        // The method pointer in the frame might be the only path to the method's
        // klass, and the klass needs to be kept alive while executing. The GCs
        // don't trace through method pointers, so the mirror of the method's klass
        // is installed as a GC root.
        f->do_oop(interpreter_frame_mirror_addr());
      ```

      So when we construct the interpreter frame, we effectively go and pull Klass::java_mirror()->resolve().

      But if you look into *native* parts of Hotspot, you would notice this:

      ```
      // Loading the java_mirror does not keep its holder alive. See Klass::keep_alive().
      inline oop Klass::java_mirror() const {
        return _java_mirror.resolve();
      }
      ```

      And Klass::keep_alive() resolves the CLD:

      ```
      // This loads and keeps the klass's loader alive.
      inline oop Klass::klass_holder() const {
        return class_loader_data()->holder();
      }

      inline void Klass::keep_alive() const {
        // Resolving the holder (a WeakHandle) will keep the klass alive until the next safepoint.
        // Making the klass's CLD handle oops (e.g. the java_mirror), safe to store in the object
        // graph and its roots (e.g. Handles).
        static_cast<void>(klass_holder());
      }
      ```

      So how do we guarantee that we are keeping the klass alive when putting it into the interpreter frame? Shouldn't we instead resolve and save CLD, not Klass in the interpreter frames? E.g.:

      ```
      diff --git a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
      index 47ef0aef2bb..6618db301d8 100644
      --- a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
      +++ b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
      @@ -578,7 +578,8 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
         // Get mirror and store it in the frame as GC root for this Method*.
         // rax is still ConstantPool*, resolve ConstantPool* -> InstanceKlass* -> Java mirror.
         __ movptr(rdx, Address(rax, ConstantPool::pool_holder_offset()));
      - __ movptr(rdx, Address(rdx, in_bytes(Klass::java_mirror_offset())));
      + __ movptr(rdx, Address(rdx, in_bytes(Klass::class_loader_data_offset())));
      + __ movptr(rdx, Address(rdx, in_bytes(ClassLoaderData::holder_offset())));
         __ resolve_oop_handle(rdx, rscratch2);
         __ push(rdx);
      ```

            Assignee:
            Coleen Phillimore
            Reporter:
            Aleksey Shipilev
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated: