8333791: Fix memory barriers for @Stable fields

[shipilev]

See bug for more discussion.

Currently, C2 puts a Release barrier at exit of every method that writes a @Stable field. This is a problem for high-performance code that initializes the stable field like this:

jdk/src/java.base/share/classes/java/lang/Enum.java

Lines 182 to 193 in 79a2301

	public final int hashCode() {
	// Once initialized, the hash field value does not change.
	// HotSpot's identity hash code generation also never returns zero
	// as the identity hash code. This makes zero a convenient marker
	// for the un-initialized value for both @Stable and the lazy
	// initialization code below.
	int hc = hash;
	if (hc == 0) {
	hc = hash = System.identityHashCode(this);
	}
	return hc;
	}

A more egregious example is here, which means that every String constructor actually does Release barrier for @Stable field write, while only a StoreStore for final field store would suffice:

jdk/src/java.base/share/classes/java/lang/String.java

Lines 159 to 160 in 79a2301

	@Stable
	private final byte[] value;

AFAICS, the original intent for Release barrier in constructor for stable fields was to match the memory semantics of final fields better. @Stable are in some sense "super-finals": they are foldable like static finals or non-static trusted finals, but can be written anywhere. The @Stable machinery is intrinsically safe under races: either a compiler sees a component of stable subgraph in initialized state and folds it, or it sees a default value for the component and leaves it alone.

I performed an audit of current @Stable uses for fields that are not currently final or volatile, and there are cases where we write into @Stable fields in constructors. AFAICS, they are covered by final-field-like semantics by accident of having adjacent final fields.

Current PR implements Variant 2 from the discussion: makes sure stable fields are as memory-safe as finals, and that's it. I believe this is all-around a good compromise for both mainline and the backports: the performance is improved in one the path that matter, and we still have some safety margin in face of accidental removals of adjacent final-s, or in case I missed some spots during the audit.

C1 did not do anything special for @Stable fields at all, fixed those to match C2. Both Zero and template interpreters for non-TSO arches put barriers at every return (with notable exception of ARM32), which handles everything in an overkill manner.

Additional testing:

• New IR tests
• Linux x86_64 server fastdebug, all
• Linux AArch64 server fastdebug, all
• Linux AArch64 server fastdebug, jcstress tests

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8333791: Fix memory barriers for @Stable fields (Enhancement - P4)

Reviewers

• Chen Liang (@liach - Reviewer)
• Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19635

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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]

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

8333791: Fix memory barriers for @Stable fields

Reviewed-by: liach, vlivanov

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 10 new commits pushed to the master branch:

• 41e31d6: 8337622: IllegalArgumentException in java.lang.reflect.Field.get
• 2ca136a: 8337815: Relax G1EvacStats atomic operations
• 99edb4a: 8337702: Use new ForwardExceptionNode to call StubRoutines::forward_exception_entry()
• 8d08314: 8337795: Type annotation attached to incorrect type during class reading
• f84240b: 8338011: CDS archived heap object support for 64-bit Windows
• 04b146a: 8337334: Test tools/javac/7142086/T7142086.java timeout with fastdebug binary
• a36fb36: 8338108: Give better error message in configure if a full XCode is missing
• 61d1dc5: 8334466: Ambiguous method call with generics may cause FunctionDescriptorLookupError
• 89a15f1: 8337681: PNGImageWriter uses much more memory than necessary
• a6c0630: 8337938: ZUtils::alloc_aligned allocates without reporting to NMT

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[openjdk]

@shipilev 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.

[rose00]

I think we should remove wrote_stable and all associated logic. The full argument is in my comment the bug.

https://bugs.openjdk.org/browse/JDK-8333791

Stable fields are in some ways “better finals”, in that they can be used to store lazy but effectively final states. But part of the “better” is that (correctly used) their race conditions are safe. Since racing is part of their nature, the fences are an unnecessary expense.

So just removing that code would be the best outcome, unless I am missing something. We will want to run such a change through heavy testing.

[mlbridge]

Webrevs

• 02: Full (52c83816)
• 01: Full (561ea2f2)
• 00: Full (3892d37f)

[liach]

Can the barrier issue be bypassed with this pattern:

private @Stable Value field;

Value getter() {
    var local = field; // avoid double read
    if (local == null)
        local = computeAndSet(); // avoid double read, no fence here in getter
    return local;
}

private Value computeAndSet() {
    var result = ... // compute value
    field = result; // write must be here, or barrier will be in getter
    return result; // to avoid double read
}

And since you are still inserting barriers the same way constructor barriers are inserted, can I say that such a more usual pattern:

private @Stable Value field;

Value getter() {
    var local = field; // avoid double read
    if (local == null)
        local = field = ... // inserts StoreStore after
    return local;
}

will still suffer from the regression observed in #19433 (comment), or is that completely fixed?

[dean-long]

Do we still need separate _wrote_stable and _wrote_final flags, or could we combine them into _wrote_stable_or_final?
Then we are almost back to pre-8031818, when _wrote_final was overloaded to mean write to final or stable field.

[liach]

If we merge the stable and final flags, won't this:

Value getter() {
    var local = field;
    if (local == null)
        local = field = ... // makes the getter final-writing
    return local;
}

be regarded the same as any final-writing constructor?

Then every call to getter() is fenced, and the issue in #19433 isn't solved at all.

[dean-long]

@liach, if I understand this PR correctly, it only adds barriers for final/stable fields in constructors. Previous code to emit barriers for stable fields outside of constructors is removed.

[liach]

Ah, so it's like a weaker version of always safe construction. Makes sense.

[rose00]

I like this compromise. Let me see if I got it right: A stable write in a constructor is treated like a final write — it triggers a barrier at the end of the constructor. That’s a cheap move. No other barriers are added automatically, for reads or other writes, saving us from doing less cheap moves. The burden would be on users of stable vars (in fancy access patterns) to add more fences if needed, but we don’t see any important cases of that, at the moment.

[shipilev]

If we merge the stable and final flags, won't this be regarded the same as any final-writing constructor?

Nope. With this patch, we only care about stable field barriers in constructors. Methods are not affected. There are new IR tests that verify this directly.

[shipilev]

Do we still need separate _wrote_stable and _wrote_final flags, or could we combine them into _wrote_stable_or_final? Then we are almost back to pre-8031818, when _wrote_final was overloaded to mean write to final or stable field.

One of my previous iterations did this combination, but I thought it was: a) uglier; b) not future-proof, in case someone (probably me, later) would like to check the parser state for final fields writes specifically. So I thought to track final and stable field writes separately.

[shipilev]

I like this compromise. Let me see if I got it right: A stable write in a constructor is treated like a final write — it triggers a barrier at the end of the constructor. That’s a cheap move. No other barriers are added automatically, for reads or other writes, saving us from doing less cheap moves. The burden would be on users of stable vars (in fancy access patterns) to add more fences if needed, but we don’t see any important cases of that, at the moment.

Yes, pretty much.

Looking at this performance/safety model another way, there is now no performance cost or safety risk for simple changes in user code like:

1. Changing a previously final field into @Stable field with value overwrite outside of constructor. This looks like a useful pattern in java.lang.invoke. There is no performance cost, as we will emit the same barrier in constructor, and emit no barriers outside of them. There is no safety risk for constructors, as the initializations would behave like finals.
2. Changing a previously @Stable field into final field, if the only stores are in constructor. Basically, the reversal of (1). No additional performance cost, since the emitted barriers would be the same.
3. Putting a @Stable over final field. This is where current String constructor gets a bad deal today: emits double barriers, one for stable, one for final. With this change, there is no performance cost anymore, a single final barrier would be emitted.
4. Putting a @Stable over any field that is written outside of constructor. This is where lazy caches like Enum.hashCode get a bad deal today. With this change, there is no performance cost anymore, since no barriers are emitted over stable stores.

Users are still responsible for proper fencing if value is overwritten outside of constructor, and the data races on the field are not benign. For example when @Stable field is used as release-acquire witness somewhere or constructor does reads of these fields for some other reason.

[shipilev]

Still waiting for formal reviews, thanks!

[liach]

/reviewers 2 reviewer

[openjdk]

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[PaulSandoz]

IIUC this means we can remove the explicit fence here:

    public ConstantCallSite(MethodHandle target) {
        super(target);
        isFrozen = true;
        UNSAFE.storeStoreFence(); // properly publish isFrozen update
    }

?

[liach]

IIUC this means we can remove the explicit fence here

ConstantCallSite is non-sealed, and we probably wish to read isFrozen == true when we can read anything initialized by the subclasses, especially if a malicious subclass leaks itself into some multithreaded environment before quitting the constructor.

That said, I think we can change this to a StoreStore or Release:

jdk/src/java.base/share/classes/java/lang/invoke/MutableCallSite.java

Line 277 in 8feabc8

STORE_BARRIER.lazySet(0);

[shipilev]

IIUC this means we can remove the explicit fence here:

    public ConstantCallSite(MethodHandle target) {
        super(target);
        isFrozen = true;
        UNSAFE.storeStoreFence(); // properly publish isFrozen update
    }

I think so, but there is more to it: there are other fences around the CallSite-s that might be related to this. I would prefer not to do it any of usage changes in this PR. Separately, I tried to benchmark new ConstantCallSite(MH) just to see if these barriers are merged, and quickly realized there is a bunch of MethodHandleNatives$CallSiteContext with Cleaners get instantiated for every CCS created, which completely dominates any wins we get from removing this fence.

[PaulSandoz]

IIUC this means we can remove the explicit fence here:

    public ConstantCallSite(MethodHandle target) {
        super(target);
        isFrozen = true;
        UNSAFE.storeStoreFence(); // properly publish isFrozen update
    }

I think so, but there is more to it: there are other fences around the CallSite-s that might be related to this. I would prefer not to do it any of usage changes in this PR.

Agreed, just wanted to test my understanding.

[shipilev]

Still waiting for formal reviews, please :)

[shipilev]

...and still looking for formal reviews, pretty please :)

[iwanowww]

Looks good.

[liach]

This updated patch to the new test framework looks clean.

I think current @Stable is a few distinct usages bundled together:

1. lazy variables or arrays - addressed by StableValue jep
2. frozen arrays - there's an inactive frozen array proposal
3. constant folding outside of trusted packages - addressed by strict final fields (nullable types jep)

I hope we can gradually roll out the 3 features to benefit all java users.

[shipilev]

Thanks! Last call for comments. If there are no other comments, I am going to integrate this with the next 24 hours.

[shipilev]

Actually, maybe anyone wants to run it through their testing pipelines as well? @iwanowww, @chhagedorn, @TobiHartmann?

[chhagedorn]

Already done, sorry forgot to report.

Ran t1-4,hs_precheckin_comp,hs_comp_stress - looked good!

[shipilev]

Awesome, thanks! Here it goes, then.

/integrate

[openjdk]

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

• da7311b: 8338286: GHA: Demote x86_32 to hotspot build only
• f536f5a: 8336086: G1: Use one G1CardSet instance for all young regions
• 4c34433: 8338304: clang on Linux - check for lld presence after JDK-8333189
• 4669e7b: 8337102: JITTester: Fix breaks in static initialization blocks
• e3a5e26: 8338344: Test TestPrivilegedMode.java intermittent fails java.lang.NoClassDefFoundError: jdk/test/lib/Platform
• aff7936: 8338333: Add jls links to javax.lang.model.element.Modifier
• 723ac57: 8332488: Add JVMTI DataDumpRequest to the debug agent
• c0384b6: 8337237: Use FFM instead of Unsafe for Java 2D RenderBuffer class
• 6a39014: 8338110: Exclude Fingerprinter::do_type from ubsan checks
• 0e3903f: 8338393: Parallel: Remove unused ParallelCompactData::clear_range
• ... and 41 more: https://git.openjdk.org/jdk/compare/03204600c596214895ef86581eba9722f76d39b3...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit 74fdd68.

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

[merykitty]

What if the field is a field of another object, not the one in construction? For final fields the verifier ensures that we only write to it in the containing object constructor, but for final fields it is not guaranteed.

[liach]

Isn't membar additions already checked with is_object_initializer() or method()->name() == ciSymbols::object_initializer_name()?

[merykitty]

Yes we only emit membars at the end of constructors but we do not check if the stable fields being written are of the same objects as the ones being constructed there.

[liach]

That should be still fine, as this shouldn't affect performance-sensitive fast paths.