8351569: [lworld] Revisit atomic access modes in flat var handles #1402

mcimadamore · 2025-03-19T18:31:27Z

This PR is an attempt to put var handle support for flat values on a more solid footing. Some more notes in a comment below.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8351569: [lworld] Revisit atomic access modes in flat var handles (Bug - P3)

Reviewers

Chen Liang (@liach - no project role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1402

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1402.diff

Using Webrev

Link to Webrev Comment

Add more tests

bridgekeeper · 2025-03-19T18:32:15Z

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

openjdk · 2025-03-19T18:32:36Z

@mcimadamore 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:

8351569: [lworld] Revisit atomic access modes in flat var handles

Reviewed-by: liach

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

mcimadamore · 2025-03-19T18:56:42Z

Problem

The VM has several different flat layouts to pick from. Some layouts support atomic load/stores (but require the value payload, including the null channel -- where present -- to fit in 64 bits) -- we call these layouts atomic layouts. Others do not.

When value fields/array elements are accessed through the VarHandle API, a question arises: under which conditions can the API support the various access modes provided by the API? Should all access modes be available, regardless of the JVM layout? Or should non-plain access modes only be supported by atomic JVM layouts?

The current implementation attempts to do the former -- all access modes are supported all the time. This is done by surrounding atomic operations such as CAS with a synchronized block. While this strategy is good for prototyping, it is not efficient, and only works correctly as long as all the accesses occur via the synchronized block. For instance, if CAS code is mixed with plain access, the CAS code might observe a torn read -- which seems bad.

Solution

The VarHandle API should only provide access to non-plain access mode if either a layout is non-flat, or if the layout is flat and atomic. In the former case, we can implement access using Unsafe's reference access primitives. In the latter case we can implement access using a combination of Unsafe::getFlatValue, memory barriers (for non-plain access modes such as acquire, or volatile) and weak CAS loops. In other words, by limiting support to atomic flat layouts, no synchronized block is ever required.

One important design consideration is that we want the set of access modes supported by a VarHandle to be stable and predictable. That is, given that there are many factors influencing which JVM layout is picked for a field or an array element, how do user knows if a CAS operation is going to fail or not?

To provide for more stability I've opted for a simple rule: a field var handle supports atomic operations if either

it is declared volatile
its type is nullable
the field type is not a @WeaklyConsistent class

For arrays, things are more convoluted: as arrays are covariant, it is always possible to create an array for Object[].class but then use it against a flat array. In such cases some dynamic dispatch is unavoidable: the array var handle code has to detect covariant cases, and switch to "sharper" code that is able to deal with the accessed array. An accessed array instance supports atomic operations if either:

ValueClass::isAtomicArray returns true for the accessed array
ValueClass::isNullRestrictedArray returns false for the accessed array
the component type of the accessed array is not a @WeaklyConsistent class

(Note the above rules mean that var handles targeting migrated value class in JEP 401 will, by definition, support all access modes -- as all uses of such classes will be through nullable types).

Changes

In the code this PR replaces, there are two VarHandle implementation classes, namely VarHandleReferences and VarHandleFlatValues. These classes support all access modes -- one is used for non-flat values, the other is used for flat values.

This first change is to refine the implementation classes so that we can support four cases:

flat values that support non-plain access modes (VarHandleFlatValues)
flat values that do not support non-plain access modes (VarHandleNonAtomicFlatValues)
non-flat values that support non-plain access modes (VarHandleReferences)
non-flat values that do not support non-plain access modes (VarHandleNonAtomicReferences)

The VarHandles::makeFieldVarHandle/makeStaticFieldVarHandle is then updated so that the correct var handle is returned, based on the characteristics of the field. As mentioned above, for VarHandles::makeArrayElementHandle our strategy is to always create an indirect var handle that supports all access modes (VarHandleReferences.Array). When an flat array is passed to the methods in this class, we will (a) check that the array is atomic and (b) use flat Unsafe access primitives..

The decision of whether a given field or array should support non-plain operation is defined in the VarHandles::isAtomicFlatpredicates (there's one for fields and one for arrays). The logic there follows what discussed above, with one exception: we only treat as atomic value types that have no object pointers in them. This is due to a technical limitation in the Unsafe support for the atomic operations (like CAS), where we can turn a flat value CAS into a numeric CAS only as long as the value payload doesn't contain objects (this restrictions prevents us from accidentally updating an object pointer without notifying the GC). While this restriction shouldn't be viewed as permanent, supporting CAS operations on flat values with pointers is a separate challenge that will not be addressed by this PR.

This PR updates most of the non-plain access primitives for flat values in the Unsafe class. Atomic operations are now implemented using weak CAS loops. We needed to perform some heroics to turn a value payload into a numeric value (a byte, a short an int or a long), so that we can turn a flat value CAS into a numeric CAS -- this logic is in Unsafe::compareAndSetFlatValueAsBytes. Non-plain, non-atomic access (e.g. opaque, acquire/release, volatile) are implemented in terms of plain access, with extra barriers -- as described in this document. This allows us to implement all the access modes on atomic flat layouts in Java. It is possible that, in the future, some (or all) of these methods will turn into JVM intrinsics which will allow to avoid some of the workarounds described here.

Finally, note that even some atomic operations involving reference types have been updated as well -- when a value type is passed to a reference CAS, we need to perform CAS in a way so that it uses the correct definition of == (e.g. not pointer comparison). Before this PR this was done, again, with a monitor. All monitor code is now gone.

Testing

The main new test added here is FlatVarHandleTest -- this checks that field and array element var handles support the expected access modes. This test doesn't focus on whether the implementation of the access primitive is correct -- only on whether the access mode is supported or not. An existing test (NullRestrictedArraysTest) has been augmented to test the correctness of the various non-plain mode accesses.

Some test cases in one JVM test (TestIntrinsics) had to be disabled: this test expects calls to atomic operations on flat values to always work -- regardless of whether such flat values feature an atomic layout or not. This is incorrect, as the new Unsafe primitives only work with atomic flat layouts (as we need to be able to turn a value layout into a numeric value).

mcimadamore · 2025-03-20T10:26:38Z

src/java.base/share/classes/java/lang/invoke/VarHandles.java

+                                    ? new VarHandleReferences.FieldInstanceReadOnly(refc, foffset, type, f.getCheckedFieldType())
+                                    : new VarHandleReferences.FieldInstanceReadWrite(refc, foffset, type, f.getCheckedFieldType()));
+                        } else {
+                            return maybeAdapt(f.isFinal() && !isWriteAllowedOnFinalFields


While this case is not terribly important (typically, if something is non-flat, then it will be "atomic" from a programming model perspective), there are various JVM flags that can affect whether something is flattened or not. It's important here that we don't return a var handles that "overpromises" and support more access modes (just because we know that, underneath, we can use reference Unsafe primitives) as such behavior might not be stable,

mlbridge · 2025-03-20T10:55:17Z

Webrevs

mcimadamore · 2025-03-20T11:11:40Z

src/java.base/share/classes/java/lang/invoke/VarHandles.java

-            return maybeAdapt(vh);
+            // do not check for atomicity now -- atomicity will be checked on the accessed array object
+            // a sharper var handle will be obtained (dynamically) from flatArrayElementHandleFor(Object)
+            return maybeAdapt(new VarHandleReferences.Array(aoffset, ashift, arrayClass));


Note: this works because:

the reference Array plain access uses array syntax (which works no matter the array passed in)

non-plain access modes always delegate to sharper var handles based on the type of the accessed array

Another note: at the language level there's no mechanism to obtain a class mirror for a null-restricted, or atomic array. This means we can't really create an array var handle that works only on one array shape (which would allow us to eliminate the dynamic dispatch completely, at least for some array var handles).

Yep, now it is crucial for us to benchmark these var handles - I suspect they are very sensitive to profile pollutions.

…lat arrays Cache result of oop-free analysis Beef up tests to check different combinations of VM flags

mcimadamore · 2025-03-20T14:06:02Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

+        // try nullable atomic array first
+        Object expectedArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        Object xArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        if (arrayLayout(expectedArray.getClass()) != layout) {


This part feels a bit dirty: we are receiving a request for layout X - we need to be able to create an array with that same layout -- but there's no primitive for doing that. Which means I have to try using different factories and make sure that I get something compatible (which isn't always guaranteed because of VM flags). All this guesswork should not be necessary -- that is, there should be an unsafe API to create an array with the layout I want (assuming that layout is supported of course)

I have considered avoiding the array creation -- but that is messy because it means you have to deal with nulls directly. E.g. if either expected or x are null, what is their corresponding numeric configuration? Going through an array element allows the code to remain agnostic as to how nulls are represented. Another way to avoid array creation would be to have some way to unsafely get the payload of a value as a long, including null channel and everything.

I think runtime support is better for down the road for this part.

We have decided to address this by adding a primitive to create a flat array with given known layout:
#1404

Once that API is integrated, I will rebase this PR on top of that.

That API is now in. This patch looks fine to me otherwise.

I've now simplified the code to use the new primitive.

mcimadamore · 2025-03-20T18:02:01Z

test/jdk/valhalla/valuetypes/NullRestrictedArraysTest.java

+
+        // test atomic set with null witness
+
+        assertFalse(vh.compareAndSet(array, 2, null, value1));


I realized that the VH code we have is liberal w.r.t. null witness values in CAS-like operations -- in the sense that it allows them, even though the underlying variable is null-restricted. For now I decided to preserve this behavior (instead e.g. throwing an IAE) - but better add a test :-)

mcimadamore · 2025-03-20T18:23:50Z

make/modules/java.base/gensrc/GensrcVarHandles.gmk

@@ -39,43 +39,77 @@ VARHANDLES_SRC_DIR := $(MODULE_SRC)/share/classes/java/lang/invoke
 # Param 2 - Type with first letter capitalized
 define GenerateVarHandle

-  $1_Type := $2
+  $1_InputType := $2


We need to do a bit of trickery here. Basically we have two pair of input values, which should use the same unsafe access primitives:

Reference, NonAtomicReference -> get/putReference

FlatValue, NonAtomicFlatValue -> get/putFlatValue

So, we now use InputType to model the input type to the template, and from there we derive Type (which is really the Unsafe access type).

There's also some other changes:

Not all generated classes need non-plain operations (e.g. NonAtomicReference, NonAtomicFlatValue do not)

Not all generated classes need to handle static fields (e.g. FlatValues and NonAtomicFlatValues do not, as static fields are never flattened)

Not all generated classes need to deal with arrays (e.g. FlatValues and NonAtomicFlatValues do not -- access to value arrays is always handled in the Reference impl class)

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

liach · 2025-03-21T15:35:37Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

+        // try nullable atomic array first
+        Object expectedArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        Object xArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        if (arrayLayout(expectedArray.getClass()) != layout) {


I think runtime support is better for down the road for this part.

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

Co-authored-by: Chen Liang <[email protected]>

liach · 2025-03-21T17:45:15Z

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

+        // try nullable atomic array first
+        Object expectedArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        Object xArray = ValueClass.newNullableAtomicArray(valueType, 1);
+        if (arrayLayout(expectedArray.getClass()) != layout) {


That API is now in. This patch looks fine to me otherwise.

liach · 2025-03-21T17:46:51Z

src/java.base/share/classes/java/lang/invoke/VarHandles.java

-            return maybeAdapt(vh);
+            // do not check for atomicity now -- atomicity will be checked on the accessed array object
+            // a sharper var handle will be obtained (dynamically) from flatArrayElementHandleFor(Object)
+            return maybeAdapt(new VarHandleReferences.Array(aoffset, ashift, arrayClass));


Yep, now it is crucial for us to benchmark these var handles - I suspect they are very sensitive to profile pollutions.

liach · 2025-03-21T17:47:21Z

src/java.base/share/classes/java/lang/invoke/X-VarHandle.java.template

@@ -433,7 +438,7 @@ final class VarHandle$Type$s {
        final CheckedType checkedFieldType;
 #end[Object]
 #if[FlatValue]
-        final int layout;
+        final int layout;        


extra spaces

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

liach · 2025-03-21T17:50:25Z

test/jdk/valhalla/valuetypes/NullRestrictedArraysTest.java

@@ -25,8 +25,7 @@
 /*
 * @test
 * @enablePreview
- * @run junit/othervm NullRestrictedArraysTest
- * @run junit/othervm -XX:-UseArrayFlattening NullRestrictedArraysTest


Why remove the no flattening configurations?

If array flattening is disabled, but field flattening is enabled, we might run into situation where the new flat array primitive fails -- that primitive requires same level of flattening for both arrays and fields.

I've now re-enabled this combo -- but disabled both array flattening and nullable flattening.

… array creation primitive

src/java.base/share/classes/jdk/internal/misc/Unsafe.java

Co-authored-by: Chen Liang <[email protected]>

liach

Looks good.

mcimadamore added 4 commits March 18, 2025 16:52

Initial push

31fb09c

Reintroduce assertion that static fields can't be flat

7bd00e1

Refactor flat value var handle implementation

e845193

Make atomic var handle support depend on user-visible properties

874e4cb

Add more tests

mcimadamore changed the title ~~Make atomic var handle support depend on user-visible properties~~ 8351569: [lworld] Revisit atomic access modes in flat var handles Mar 20, 2025

mcimadamore commented Mar 20, 2025

View reviewed changes

mcimadamore added 2 commits March 20, 2025 10:47

Fix missing fence in putFlatValueVolatile

d4af8c1

Remove redundant import

856cb9a

mcimadamore marked this pull request as ready for review March 20, 2025 10:51

openjdk bot added ready rfr labels Mar 20, 2025

mcimadamore commented Mar 20, 2025

View reviewed changes

Enhance compareAndSetFlatValueAsBytes to work on different kinds of f…

8ca6bef

…lat arrays Cache result of oop-free analysis Beef up tests to check different combinations of VM flags

mcimadamore commented Mar 20, 2025

View reviewed changes

mcimadamore added 2 commits March 20, 2025 16:36

Fix test header

1cd8176

Avoid dynamic var handle creation in the middle of array element access

70af9a2

mcimadamore commented Mar 20, 2025

View reviewed changes

Tighten the signature of VarHandles::checkAtomicFlatArray

0e2eff2

mcimadamore commented Mar 20, 2025

View reviewed changes

liach reviewed Mar 21, 2025

View reviewed changes

mcimadamore and others added 3 commits March 21, 2025 16:14

Update src/java.base/share/classes/jdk/internal/misc/Unsafe.java

b87221a

Co-authored-by: Chen Liang <[email protected]>

Update src/java.base/share/classes/jdk/internal/misc/Unsafe.java

7ddd549

Co-authored-by: Chen Liang <[email protected]>

Add missing @ForceInline

6b1b379

liach reviewed Mar 21, 2025

View reviewed changes

mcimadamore added 2 commits March 21, 2025 18:37

Merge branch 'lworld' into varhandle_flat

be6f00d

Simplify Unsafe::compareAndSetFlatValueAsBytes to use new unsafe flat…

f8c7d9b

… array creation primitive

Address review commments

000917a

liach reviewed Mar 21, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/misc/Unsafe.java Outdated Show resolved Hide resolved

Update src/java.base/share/classes/jdk/internal/misc/Unsafe.java

a823ba4

Co-authored-by: Chen Liang <[email protected]>

liach approved these changes Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8351569: [lworld] Revisit atomic access modes in flat var handles #1402

8351569: [lworld] Revisit atomic access modes in flat var handles #1402

mcimadamore commented Mar 19, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 19, 2025

openjdk bot commented Mar 19, 2025 •

edited

Loading

mcimadamore commented Mar 19, 2025 •

edited

Loading

mcimadamore Mar 20, 2025

mlbridge bot commented Mar 20, 2025 •

edited

Loading

mcimadamore Mar 20, 2025

mcimadamore Mar 20, 2025

liach Mar 21, 2025

mcimadamore Mar 20, 2025

mcimadamore Mar 20, 2025 •

edited

Loading

liach Mar 21, 2025

mcimadamore Mar 21, 2025

liach Mar 21, 2025

mcimadamore Mar 21, 2025

mcimadamore Mar 20, 2025 •

edited

Loading

mcimadamore Mar 20, 2025

liach Mar 21, 2025

liach Mar 21, 2025

liach Mar 21, 2025

liach Mar 21, 2025

liach Mar 21, 2025

mcimadamore Mar 21, 2025

mcimadamore Mar 21, 2025

liach left a comment


		// test atomic set with null witness

		assertFalse(vh.compareAndSet(array, 2, null, value1));

8351569: [lworld] Revisit atomic access modes in flat var handles #1402

Are you sure you want to change the base?

8351569: [lworld] Revisit atomic access modes in flat var handles #1402

Conversation

mcimadamore commented Mar 19, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 19, 2025

openjdk bot commented Mar 19, 2025 • edited Loading

mcimadamore commented Mar 19, 2025 • edited Loading

Problem

Solution

Changes

Testing

Choose a reason for hiding this comment

mlbridge bot commented Mar 20, 2025 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

mcimadamore commented Mar 19, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 19, 2025 •

edited

Loading

mcimadamore commented Mar 19, 2025 •

edited

Loading

mlbridge bot commented Mar 20, 2025 •

edited

Loading

mcimadamore Mar 20, 2025 •

edited

Loading

mcimadamore Mar 20, 2025 •

edited

Loading