8299032: Interface IN_NATIVE oop stores for C2 #11777

fisk · 2022-12-23T13:54:44Z

Loom added the first IN_NATIVE oop store to C2 code, when switching the current virtual thread of a platform thread. Instead of interfacing this properly, a raw store was used. But a future GC algorithm, such as generational ZGC, might not work well with raw stores. We should add support to the access API for this. Looks like Shenandoah was already missing something here as they do concurrent root processing requiring SATB barriers on IN_NATIVE oop stores. It is not in the scope of this PR to fix the Shenandoah bug - someone working on Shenandoah should do this. This PR merely adds an interface such that GCs that need to do something different here, can do that. Serial, Parallel and G1 don't need to do anything for IN_NATIVE stores as they do STW root processing. ZGC doesn't (yet) have store barriers at all, and hence doesn't need to do anything special either.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8299032: Interface IN_NATIVE oop stores for C2

Reviewers

Stefan Karlsson (@stefank - Reviewer)
Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11777/head:pull/11777
$ git checkout pull/11777

Update a local copy of the PR:
$ git checkout pull/11777
$ git pull https://git.openjdk.org/jdk pull/11777/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11777

View PR using the GUI difftool:
$ git pr show -t 11777

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11777.diff

bridgekeeper · 2022-12-23T13:55:57Z

👋 Welcome back eosterlund! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-12-23T13:58:36Z

@fisk The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-12-23T14:02:29Z

Webrevs

00: Full (e5cdc183)

fisk · 2023-01-09T10:45:29Z

Thanks for the review, @stefank!

openjdk · 2023-01-09T10:48:13Z

@fisk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8299032: Interface IN_NATIVE oop stores for C2

Reviewed-by: stefank, rcastanedalo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 268 new commits pushed to the master branch:

48c8fb3: 8299978: Remove MethodHandleNatives.getMembers
89d3f3d: 8299125: UnifiedOopRef in JFR leakprofiler should treat thread local oops correctly
4b57334: 8278326: Socket close is not thread safe and other cleanup
036c808: 8298482: Implement ParallelGC NUMAStats for Linux
0ee8cac: 8299672: Enhance HeapDump JFR event
d716ec5: 8299179: ArrayFill with store on backedge needs to reduce length by 1
af8d3fb: 8264684: os::get_summary_cpu_info padded with spaces
78b1686: 8227257: javax/swing/JFileChooser/4847375/bug4847375.java fails with AssertionError
33f3bd8: Merge
945ef07: 8299862: OfAddress setter should disallow heap segments
... and 258 more: https://git.openjdk.org/jdk/compare/fa322e40b68abf0a253040d14414d41f4e01e028...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kimbarrett

Not a review, since I'm not sufficiently knowledgeable about C2 to really do justice to it.
Rather, a couple questions/comments.

We made a concerted effort to reduce/eliminate oops in native storage, instead using OopStorage entries. This kind of feels like backsliding on that effort. Why?

kimbarrett · 2023-01-10T01:56:11Z

src/hotspot/share/opto/library_call.cpp

-  // Stores of oops to native memory not supported yet by BarrierSetC2::store_at_resolved
-  // access_store_at(NULL, thread_obj_handle, adr_type, arr, _gvn.type(arr), T_OBJECT, IN_NATIVE | MO_UNORDERED);
-  store_to_memory(control(), thread_obj_handle, arr, T_OBJECT, adr_type, MemNode::unordered);
+  access_store_at(NULL, thread_obj_handle, adr_type, arr, _gvn.type(arr), T_OBJECT, IN_NATIVE | MO_UNORDERED);


Old code has a control() constraint on the store. Is it okay to drop that?

The first parameter to access_store_at, is the containing object, not the control (which might be different to the address). As for IN_NATIVE stores, there is no containing object, which is what the NULL denotes. In the backend, it does indeed attach the current control inside of BarrierSetC2::store_at_resolved.

Thanks for the explanation.

fisk · 2023-01-10T09:37:01Z

Not a review, since I'm not sufficiently knowledgeable about C2 to really do justice to it. Rather, a couple questions/comments.

We made a concerted effort to reduce/eliminate oops in native storage, instead using OopStorage entries. This kind of feels like backsliding on that effort. Why?

It isn't a backsliding on that effort. This code is indeed using OopStorage. An OopHandle::replace uses IN_NATIVE stores. This is essentially an intrinsic that replaces the contents of an OopHandle, so similarly it should use an IN_NATIVE store for that. Having said that, perhaps it would have been more clear if there was a high level function of some sort with a name like oop_handle_replace containing the IN_NATIVE store?

kimbarrett · 2023-01-12T07:01:18Z

Not a review, since I'm not sufficiently knowledgeable about C2 to really do justice to it. Rather, a couple questions/comments.
We made a concerted effort to reduce/eliminate oops in native storage, instead using OopStorage entries. This kind of feels like backsliding on that effort. Why?

It isn't a backsliding on that effort. This code is indeed using OopStorage. An OopHandle::replace uses IN_NATIVE stores. This is essentially an intrinsic that replaces the contents of an OopHandle, so similarly it should use an IN_NATIVE store for that. Having said that, perhaps it would have been more clear if there was a high level function of some sort with a name like oop_handle_replace containing the IN_NATIVE store?

Oh, I see now. The make_load is smashing through the OopHandle abstraction. That's of course not new to
this change.

robcasloz

Looks good! Do not forget to update the copyright headers.

I checked the coverage of inline_native_setCurrentThread() in tiers 1-3, and this function is only exercised by the serviceability and a few long-running, CTW-like test cases. We should probably extend test/hotspot/jtreg/compiler/intrinsics with specific tests for Loom intrinsics (in a separate RFE).

fisk · 2023-01-12T15:29:07Z

Thanks for the review, @robcasloz!

fisk · 2023-01-13T12:45:51Z

/integrate

openjdk · 2023-01-13T12:48:41Z

Going to push as commit e7fa150.
Since your change was applied there have been 282 commits pushed to the master branch:

c250222: 8300110: Unproblemlist Fuzz.java from ProblemList-zgc.txt
ac63f5f: 8297877: Risk for uninitialized memory in case of CHECK macro early return as part of field access
be8e6d0: 8299957: Enhance error logging in instrument coding with additional jplis_assert_msg
640eff6: 8300040: TypeOopPtr::make_from_klass_common calls itself with args in wrong order
19628e3: 8300068: UBSan CFLAGS/LDFLAGS not passed when building ADLC
9887047: Merge
4b92fb0: 8299918: Update Xcode11.3.1-MacOSX10.15 devkit at Oracle
6a4a874: 8299034: Runtime::exec clarification of inherited environment
752a370: 8299439: java/text/Format/NumberFormat/CurrencyFormat.java fails for hr_HR
3918f9f: 8299090: Specify coordinate order for additional CaptureCallState parameters on class as well
... and 272 more: https://git.openjdk.org/jdk/compare/fa322e40b68abf0a253040d14414d41f4e01e028...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-13T12:49:05Z

@fisk Pushed as commit e7fa150.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8299032: Interface IN_NATIVE oop stores for C2

e5cdc18

openjdk bot added the rfr Pull request is ready for review label Dec 23, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 23, 2022

stefank approved these changes Jan 9, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 9, 2023

kimbarrett reviewed Jan 10, 2023

View reviewed changes

robcasloz approved these changes Jan 12, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 13, 2023

openjdk bot closed this Jan 13, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299032: Interface IN_NATIVE oop stores for C2 #11777

8299032: Interface IN_NATIVE oop stores for C2 #11777

fisk commented Dec 23, 2022 •

edited by openjdk bot

bridgekeeper bot commented Dec 23, 2022

openjdk bot commented Dec 23, 2022

mlbridge bot commented Dec 23, 2022

fisk commented Jan 9, 2023

openjdk bot commented Jan 9, 2023 •

edited

kimbarrett left a comment

kimbarrett Jan 10, 2023

fisk Jan 10, 2023

kimbarrett Jan 12, 2023

fisk commented Jan 10, 2023

kimbarrett commented Jan 12, 2023

robcasloz left a comment

fisk commented Jan 12, 2023

fisk commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

8299032: Interface IN_NATIVE oop stores for C2 #11777

8299032: Interface IN_NATIVE oop stores for C2 #11777

Conversation

fisk commented Dec 23, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 23, 2022

openjdk bot commented Dec 23, 2022

mlbridge bot commented Dec 23, 2022

Webrevs

fisk commented Jan 9, 2023

openjdk bot commented Jan 9, 2023 • edited

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett Jan 10, 2023

Choose a reason for hiding this comment

fisk Jan 10, 2023

Choose a reason for hiding this comment

kimbarrett Jan 12, 2023

Choose a reason for hiding this comment

fisk commented Jan 10, 2023

kimbarrett commented Jan 12, 2023

robcasloz left a comment

Choose a reason for hiding this comment

fisk commented Jan 12, 2023

fisk commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

openjdk bot commented Jan 13, 2023

fisk commented Dec 23, 2022 •

edited by openjdk bot

openjdk bot commented Jan 9, 2023 •

edited