JDK-8246015 : Method::link_method is called twice for CDS methods
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 15
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • Submitted: 2020-05-27
  • Updated: 2022-11-01
  • Resolved: 2022-11-01
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 17
17Resolved
Related Reports
Duplicate :  
JDK-8263002 - Remove CDS MiscCode region
Relates :  
JDK-8028497 - SIGSEGV at ClassLoaderData::oops_do(OopClosure*, KlassClosure*, bool)
Relates :  
JDK-8232222 - Set state to 'linked' when an archived boot class is restored at runtime
Description
It's called once here:

http://hg.openjdk.java.net/jdk/jdk/file/0702191777c9/src/hotspot/share/oops/method.cpp#l1255

void Method::restore_unshareable_info(TRAPS) {
  assert(is_method() && is_valid_method(this), "ensure C++ vtable is restored");

  // Since restore_unshareable_info can be called more than once for a method, don't
  // redo any work.
  if (adapter() == NULL) {
    methodHandle mh(THREAD, this);
    link_method(mh, CHECK);
  }
}

and once here:

http://hg.openjdk.java.net/jdk/jdk/file/0702191777c9/src/hotspot/share/oops/instanceKlass.cpp#l911

InstanceKlass::link_class_impl() -> InstanceKlass::link_methods() -> Method::link_method()
Comments
Closing as a duplicate of JDK-8263002 . Only issue with an associated changeset are marked as "Fixed".
01-11-2022
The call Method::restore_unshareable_info() -> link_method() has been removed in JDK-8263002.
01-11-2022
> // relocate jsrs and link methods after they are all rewritten Aside, in instanceKlass.cpp, this comment is wrong. This doesn't relocate JSRs. >// Now relocate and link method entry points after class is rewritten. >// This is outside is_rewritten flag. In case of an exception, it can be >// executed more than once. >void InstanceKlass::link_methods(TRAPS) { This comment says why it's where it is. We should remove this redundant link_methods for this bug, and for JDK-8232222 and the larger proposal JDK-8233887, find a place to put it (perhaps under the conditional where you set the class to is_linked, which likely has to also check whether all the superclasses and types are already linked for valhalla).
17-06-2020
I went back to the old code and I know why we call link_method twice. The old code used to call link_method() at the end of the rewriter, but if rewriting fails we don't want any methods to already be linked because we have to reverse the rewriting: https://bugs.openjdk.java.net/browse/JDK-7033141?focusedCommentId=12449132&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-12449132 https://bugs.openjdk.java.net/browse/JDK-7033141 In doing so, I moved link_methods out of the is_rewritten() condition in link_class_impl(). Loading a class from the shared archive required calling link_methods because the shared class was already rewritten (is_rewritten == true). So this fix had the unintended consequence of linking the methods twice.
17-06-2020
[~jiangli] In Valhalla, value types can be passed by value instead of by reference. For example, if you have this class inline class Point { int x, y; } and a method like class Foo { m(Point p1, Point p2)l } Foo can be effectively invoked as : Foo::m(p1.x, p1.y, p2.x, p2.y); I.e., we don't pass the pointers p1 and p2 anymore. In order to handle this calling convention, we generate a special method entry for Foo::m has that takes in 4 integers instead of 2 pointers. However, in order to generate this special entry, we must know what fields are in the Point class. This means we need to resolve the Point type when generating the method entry. However, as you already have found out (from verification of shared classes), it's generally unsafe to resolve arbitrary types inside InstanceKlass::restore_unsharable_info(). That's why in the Valhalla repo, the method entry generation for shared classes are done during class linking, which is a stage in the class loading process that's safe for resolving arbitrary types. This is also the same place where method entries for dynamically-loaded classes are generated. ===== As far as JDK-8232222 is concerned, I wouldn't call it "planned". It's part of your larger proposal JDK-8233887, which has an elaborate design that has not been discussed in the Java community. I don't think such a proposal should stop us from fixing small issues like this one, in a way that's appropriate for the current code base and known future evolution such as Valhalla. If your proposal is indeed discussed and accepted, and indeed would require linking methods during InstanceKlass::restore_unsharable_info(), I am sure we can implement that easily, regardless of how this particular issue is fixed today.
17-06-2020
Thanks, this is related to JDK-8232222 and affects the work for pre-linking, pre-initialization and pre-resolving. As everything is interconnected, it's important to make sure this one is done right. If Valhalla changes how Method::link_method() works, I should understand why and see how that would affect the planned pre-linking, pre-initialization and pre-resolving work. So I'm reassigning the bug back to me, so I can fully understand and resolve this issue properly.
17-06-2020
Let me take over this bug. We need to discuss with compiler team and valhalla team about the proper way to fix it.
16-06-2020
Regarding JDK-8169867 "Method::restore_unshareable_info() does not invoke Method::link_method()" The bug is more about "Method::restore_unshareable_info() tries to call Method::link_method() but somehow fails to do it because it checks the wrong condition". I.e., void Method::restore_unshareable_info(TRAPS) { - if (_from_compiled_entry == NULL) { + if (adapter() == NULL) { methodHandle mh(THREAD, this); link_method(mh, CHECK); } } However, JDK-8169867 doesn't establish that "it's necessary for Method::restore_unshareable_info() to call Method::link_method()"
14-06-2020
The original call Method::restore_unshareable_info -> link_method was added in JDK-8028497: http://hg.openjdk.java.net/jdk/jdk/rev/536c66fc43d3 http://hg.openjdk.java.net/jdk/jdk/annotate/536c66fc43d3/hotspot/src/share/vm/oops/method.cpp#l906 This was subsequently modified in JDK-8169867: http://hg.openjdk.java.net/jdk/jdk/rev/14af45789042 http://hg.openjdk.java.net/jdk/jdk/annotate/14af45789042/hotspot/src/share/vm/oops/method.cpp#l1104
14-06-2020
In Valhalla, link_method can no longer be called inside Method::restore_unshareable_info(): https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/oops/method.cpp#L1300 void Method::restore_unshareable_info(TRAPS) { #if 0 /* * CDS:TODO -- * "Q" classes in the method signature must be resolved during link_method. * However, at this point we are still inside method_holder()->restore_unshareable_info. * If we try to resolve method_holder(), or multually dependent classes, it will * cause deadlock and other ill effects. ... */ if (adapter() == NULL) { methodHandle mh(THREAD, this); link_method(mh, CHECK); } #endif So, for forward compatibility, it's better to remove the call to link_method() from Method::restore_unshareable_info. It doesn't seem to do anything useful.
14-06-2020
More historical data: http://hg.openjdk.java.net/jdk/jdk/annotate/489c9b5090e2/hotspot/src/share/vm/classfile/systemDictionary.cpp#l1169 This was originally done in SystemDictionary::load_shared_class() and only subsequently refactored and moved to Method::restore_unshareable_info() for (int index2 = 0; index2 < num_methods; ++index2) { methodHandle m(THREAD, methodOop(methods->obj_at(index2))); m()->link_method(m, CHECK_(nh)); } It's unclear why the methods must be linked when the shared class is loaded. This code is from Mercurial version #1 so it was there since source code was transferred from SCCS.
14-06-2020
If we remove the call from Method::restore_unshareable_info(), there are 10 fewer calls to AdapterHandlerTable::new_entry() when running "java -version " (718 vs 728 calls). This results in about 274530 fewer instructions. However, as mentioned in the Description, this may not be doable because of JDK-8169867. Results of " perf stat -r 100 bin/java -Xshare:on -XX:SharedArchiveFile=jdk2.jsa -Xint -version " 1: 58144674 57881706 (-262968) ---- 40.622 40.387 ( -0.235) - 2: 58152688 57861647 (-291041) ----- 40.365 40.568 ( 0.203) + 3: 58151977 57869217 (-282760) ----- 40.585 40.347 ( -0.238) - 4: 58151172 57875978 (-275194) ----- 40.726 39.694 ( -1.032) ----- 5: 58131667 57872577 (-259090) ---- 41.059 40.417 ( -0.642) --- 6: 58140159 57869266 (-270893) ----- 40.726 39.841 ( -0.885) ---- 7: 58151229 57854999 (-296230) ----- 39.891 40.365 ( 0.474) ++ 8: 58137824 57880324 (-257500) ---- 40.306 39.267 ( -1.039) ----- 9: 58154187 57874158 (-280029) ----- 40.143 40.076 ( -0.067) 10: 58140863 57871263 (-269600) ----- 40.109 40.041 ( -0.068) ============================================================ 58145643 57871112 (-274530) ----- 40.452 40.098 ( -0.353) -- instr delta = -274530 -0.4721% time delta = -0.353 ms -0.8735%
27-05-2020
I'll take this. Thanks for filing the separate bug.
27-05-2020
I addressed this as the initial change for JDK-8232222: http://cr.openjdk.java.net/~jiangli/8232222/weberv.02/src/hotspot/share/oops/instanceKlass.cpp.frames.html I removed it from the latest version (yesterday's), so JDK-8232222 only handles the 'link' state setting for archived boot class: http://cr.openjdk.java.net/~jiangli/8232222/webrev.02/src/hotspot/share/oops/instanceKlass.cpp.frames.html
27-05-2020