-
Notifications
You must be signed in to change notification settings - Fork 6.3k
8330465: Stable Values (Preview) #19625
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
Conversation
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@minborg The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
src/java.base/share/classes/jdk/internal/lang/StableValues.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Chen Liang <[email protected]>
Co-authored-by: Chen Liang <[email protected]>
test/micro/org/openjdk/bench/java/lang/stable/CustomCachingPredicateBenchmark.java
Show resolved
Hide resolved
test/micro/org/openjdk/bench/java/lang/stable/CustomCachingPredicateBenchmark.java
Outdated
Show resolved
Hide resolved
After some consideration and benchmarking, we think it is better to use pure For all supported platforms, this does not incur any non-fixable significant performance regressions. It also allows us to promise real happens-before relations between stable value operations. The implementation becomes much simpler and easier to review. |
// This should never happen | ||
throw new InternalError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can happen when the mapper recursively calls the get
method on this list.
// This should never happen | |
throw new InternalError( | |
throw new ConcurrentModificationException( |
or
// This should never happen | |
throw new InternalError( | |
throw new IllegalStateException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, if the mapper is recursive, we never reach the stable.trySet()
statement. I've added a test to make sure this is the case:
@Test
void recursiveCall() {
AtomicReference<IntFunction<Integer>> ref = new AtomicReference<>();
var lazy = StableValue.lazyList(SIZE, i -> ref.get().apply(i));
ref.set(lazy::get);
assertThrows(StackOverflowError.class, () -> lazy.get(INDEX));
}
Is there another way we could end up there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following makes it happen:
var mapper = StableValue.<IntFunction<Integer>>newInstance();
var list = StableValue.lazyList(SIZE, i -> mapper.orElseThrow().apply(i));
var recursing = new boolean[SIZE];
mapper.setOrThrow(i -> {
if (recursing[i]) {
return i;
}
recursing[i] = true;
return list.get(i);
});
list.get(INDEX);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh. But that is cheating.... ;-) Well, I see what you mean now. I will update the exception type.
default void setOrThrow(T value) { | ||
if (!trySet(value)) { | ||
throw new IllegalStateException("Cannot set the holder value to " + value + | ||
" because a holder value is alredy set: " + this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" because a holder value is alredy set: " + this); | |
" because a holder value is already set: " + this); |
interestingly, when caching, after initialization, you can remove the original supplier = null. But I don't know how the @stable annotation works. But this optimization can be useful for reducing the size of the object (if no one else holds the supplier) |
|
||
* *brittleness* - the convoluted nature of the code required to write a correct double-checked locking makes it all too easy for developers to make subtle mistakes. A very common one is forgetting to add the `volatile` keyword to the `logger` field. | ||
* *lack of expressiveness* - even when written correctly, the double-checked idiom leaves a lot to be desired. The "at-most-once" mutation guarantee is not explicitly manifest in the code: after all the `logger` field is just a plain mutable field. This leaves important semantics gaps that is impossible to plug. For example, the `logger` field can be accidentally mutated in another method of the `Application` class. In another example, the field might be reflectively mutated using `setAccessible`. Avoiding these pitfalls is ultimately left to developers. | ||
* *lack of optimizations* - as the `logger` field is updated at most once, one might expect the JVM to optimize access to this field accordingly, e.g. by [constant-folding](https://en.wikipedia.org/wiki/Constant_folding) access to an already-initialized `logger` field. Unfortunately, since `logger` is just a plan mutable field, the JVM cannot trust the field to never be updated again. As such, access to at-most-once fields, when realized with double-checked locking is not as efficient as it could be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a
planplain mutable field
@minborg This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Keep alive |
@minborg What's the current status of this |
I think we are looking for 25. There is no point of internal use in java.base, but it might help other modules. For the specific issue you've linked, I think a lazy holder pattern may be sufficient. |
Stable Values (Preview)
Summary
This PR proposes to introduce an internal Stable Values & Collections API, which provides immutable value holders where elements are initialized at most once. Stable Values offer the performance and safety benefits of final fields while offering greater flexibility as to the timing of initialization.
Goals
For more details, see the draft JEP: https://openjdk.org/jeps/8312611
Performance
Performance compared to instance variables using two
AtomicReference
and two protected by double-checked locking under concurrent access by all threads:Performance compared to static variables protected by
AtomicReference
, class-holder idiom holder, and double-checked locking (all threads):All figures above are from local tests on a Mac M1 laptop and should only be constructed as indicative figures.
Implementation details
There are some noteworthy implementation details in this PR:
final StableValue
. Previously, the determination of trustworthiness was connected to the class in which it was declared (e.g. is it arecord
or a hidden class). In order to grant such trust, there are extra restrictions imposed on reflection andsun.misc.Unsafe
usage for such declaredStableValue
fields. This is similar to howrecord
classes are handled.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19625/head:pull/19625
$ git checkout pull/19625
Update a local copy of the PR:
$ git checkout pull/19625
$ git pull https://git.openjdk.org/jdk.git pull/19625/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19625
View PR using the GUI difftool:
$ git pr show -t 19625
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19625.diff