JDK-8238190 : [JVMCI] Fix single implementor speculation for diamond shapes.
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 11,15
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2020-01-29
  • Updated: 2020-06-04
  • Resolved: 2020-02-04
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 11 JDK 13 JDK 14 JDK 15
11.0.8-oracleFixed 13.0.4Fixed 14.0.2Fixed 15 b09Fixed
Related Reports
Duplicate :  
JDK-8238159 - Single implementor logic broken for diamond shapes.
Description
Single implementor speculation: ensure HotSpot supports diamond shapes.
Comments
Fix request (13u) I would like to downport this. Test fails before patch and passes with patch. Tier1 passed. Applies clean.
04-06-2020
Fix request (11u) -- will label after testing completed. I would like to downport this for parity with 11.0.8-oracle. Applies clean.
17-03-2020
Fix Request (JDK 14.0.2) The patch fixes a problem in the single implementor logic and has already been backported to JDK 11u. The fix is low risk and applies cleanly to JDK 14.0.2. In addition to CI testing through all tiers in JDK 15 and 11u, additional testing will be executed in JDK 14.0.2 before pushing.
06-03-2020
Changeset: 135f2098 Author: David Leopoldseder <david.leopoldseder@oracle.com> Committer: Doug Simon <dnsimon@openjdk.org> Date: 2020-02-04 09:33:09 +0000 URL: https://git.openjdk.java.net/panama-foreign/commit/135f2098
07-02-2020
URL: https://hg.openjdk.java.net/jdk/jdk/rev/9b3d5cc71cea User: dnsimon Date: 2020-02-04 08:40:45 +0000
04-02-2020
From JDK-8238159 closed as duplicate (I missed it): InstanceKlass records an _implementor field for classes implementing a given interface. The field's value has 3 states: (1) NULL - no implementor (2) implementor Klass* - one implementor (3) self - more than one implementor The field is initialized with NULL and set the first time an implementor is added. The next time an implementor is added _implementor already has a value != self indicating there was already a different implementor so the logic moves to state (3). This logic overlooks diamond shapes in interface class hierarchies of the following form: interface I { void foo();} interface I1 extends I {} interface I2 extends I {} static class Impl implements I1, I2 { @Override public void foo() {...} } There are 2 paths in the class hierarchy leading to I. Thus, the current logic wrongly detects this to be case (3) since it does not check if the klass in case (2) was already seen before. The fix is extending case (2) with logic checking if the value of the _implementor field is the same as the currently added implementor. This bug was revealed in Graal where we use the single implementor logic for invokeinterface devirtualization.
29-01-2020
Comments from JVMCI GR: At least single implementor based devirtualization also fails for regular java code in such a pattern public class InterfaceSingleImplementorTest extends GraalCompilerTest { static interface I { void foo(); } static interface I1 extends I { } static interface I2 extends I { } static class Impl implements I1, I2 { @Override public void foo() { GraalDirectives.sideEffect(); } } public static void snippet(I i) { i.foo(); } @Test public void test0() { // accumulate a profile for (int i = 0; i < 10000; i++) { snippet(new Impl()); } test("snippet", supply(() -> new Impl())); }} ---------------------------------------- David Leopoldseder added a comment - 2 days ago funky one, thanks Roland Schatz for the help, we did some sulong java pair debugging Seems the culprit is the hotspot code itself in InstanceKlass::add_implementor where the iklass stores an implementor field based on the documented semantics // The embedded _implementor field can only record one implementor. // When there are more than one implementors, the _implementor field // is set to the interface Klass* itself. Following are the possible // values for the _implementor field: // NULL - no implementor // implementor Klass* - one implementor // self - more than one implementor // // The _implementor field only exists for interfaces. with the following code later .... Klass* ik = implementor(); if (ik == NULL) { set_implementor(k); } else if (ik != this) { // There is already an implementor. Use itself as an indicator of // more than one implementors. set_implementor(this); } .... so every time we see another implementor we overwrite the field, however this is also done if we see the same implementor twice transitively over a diamond shape like the one above for the test case we used some stdout debugging and changed the code to work consider diamond hierarchies by checking the already stored implementor against the new one. Klass* ik = implementor(); if (ik == NULL) { set_implementor(k); } else if (ik != this && ik != k) { // There is already an implementor. Use itself as an indicator of // more than one implementors. set_implementor(this); } Tom Rodriguez can you think of a case why that would not be allowed? This would fix the implementor logic for the test case above and also the original sulong bug. -------------------------- tom.rodriguez@oracle.com Tom Rodriguez added a comment - 2 days ago No that seems reasonable to me. Just looks like an oversight.
29-01-2020