Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JDK20] Thread.findScopedValueBindings to support JEP 429? #16677

Closed
babsingh opened this issue Feb 7, 2023 · 7 comments · Fixed by #18255
Closed

[JDK20] Thread.findScopedValueBindings to support JEP 429? #16677

babsingh opened this issue Feb 7, 2023 · 7 comments · Fixed by #18255
Assignees
Labels
comp:vm jdk20 jdk21 project:loom Used to track Project Loom related work

Comments

@babsingh
Copy link
Contributor

babsingh commented Feb 7, 2023

For Thread.findScopedValueBindings,

@babsingh babsingh added the jdk20 label Feb 7, 2023
@babsingh babsingh changed the title [JDK20] Implement Thread.findScopedValueBindings to support JEP 429 [JDK20] Thread.findScopedValueBindings to support JEP 429? Feb 7, 2023
@babsingh babsingh added this to the Java 20 milestone Feb 7, 2023
babsingh added a commit to babsingh/openj9 that referenced this issue Feb 14, 2023
In JDK20, extentLocalCache is renamed to scopedValueCache.

JDK19 and JDK20 changes are completely segregated to avoid
cross-contamination if the implementation differs a lot across
JDK versions.

TODO: Thread.findScopedValueBindings needs to be implemented.
This is tracked in eclipse-openj9#16677.

Fixes: eclipse-openj9#16657

Signed-off-by: Babneet Singh <[email protected]>
@babsingh
Copy link
Contributor Author

babsingh commented Apr 24, 2023

Pseudo-code implementation:

walkStackFrames:
   for each frame:
      if ((methodName is "runWith")
      and (callerClass is either
              jdk.incubator.concurrent.ScopedValue$Carrier OR
              java.lang.VirtualThread OR
              java.lang.Thread)
      ) {
         Check if the first parameter is an jdk.incubator.concurrent.ScopedValue$Snapshot
         Then, return the first parameter as a local ref;
      )

@thallium
Copy link
Contributor

@gacholio FYI

@thallium
Copy link
Contributor

Currently we don't have a test to validate the implementation. Maybe we should put an assertion in the implementation so we will know when this is actually called.

@babsingh
Copy link
Contributor Author

Closing. An assertion has been added in #17451. Thread.findScopedValueBindings is is unused since there is no API or test that invokes this method in OpenJ9. If the assertion ever triggers, this issue should be re-opened and the draft implementation in #17402 should be completed and merged.

@babsingh babsingh reopened this Jul 17, 2023
@babsingh babsingh modified the milestones: Java 20.0.2, Java 21 Jul 17, 2023
@babsingh babsingh added the jdk21 label Jul 17, 2023
@babsingh
Copy link
Contributor Author

babsingh commented Jul 27, 2023

@0xdaryl The three runWith methods, where we want to retrieve the first argument while stack walking, are:

Issue: JIT inlining will prevent us to find the first argument during the stack walk since the O-slots don't retain their order when the methods are inlined.

#17402 contains the current implementation of the behaviour described in #16677 (comment).

@gacholio
Copy link
Contributor

For the record, you can't reliably retrieve the first argument for any compiled method, inlined or not.

@babsingh
Copy link
Contributor Author

babsingh commented Jul 27, 2023

For the record, you can't reliably retrieve the first argument for any compiled method, inlined or not.

In this case, we can look at all the arguments of the runWith methods. These methods have two arguments, and there is supposed to be only one argument of instance jdk.incubator.concurrent.ScopedValue$Snapshot.

thallium added a commit to thallium/openj9-openjdk-jdk21 that referenced this issue Aug 17, 2023
thallium added a commit to thallium/openj9-openjdk-jdk that referenced this issue Aug 18, 2023
thallium added a commit to thallium/openj9-openjdk-jdk21 that referenced this issue Aug 18, 2023
@fengxue-IS fengxue-IS added the project:loom Used to track Project Loom related work label Sep 16, 2023
babsingh pushed a commit to babsingh/openj9 that referenced this issue Oct 16, 2023
Walk the stack frames to look for the runWith() method and get
its first argument, which should be an instance of
java.lang.ScopedValue$Snapshot.

Closes: eclipse-openj9#16677

Closes: eclipse-openj9#17402

Co-authored-by: Gengchen Tuo [email protected]
Signed-off-by: Babneet Singh [email protected]
midronij pushed a commit to midronij/openj9 that referenced this issue Oct 26, 2023
Walk the stack frames to look for the runWith() method and get
its first argument, which should be an instance of
java.lang.ScopedValue$Snapshot.

Closes: eclipse-openj9#16677

Closes: eclipse-openj9#17402

Co-authored-by: Gengchen Tuo [email protected]
Signed-off-by: Babneet Singh [email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm jdk20 jdk21 project:loom Used to track Project Loom related work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants