RFR: 8264032: Improve thread safety of Runtime.version()
Avoid double-read non volatile static field ------------- Commit messages: - [PATCH] Thread-safe initialization of Runtime.version Changes: https://git.openjdk.java.net/jdk/pull/2691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2691&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264032 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2691/head:pull/2691 PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 13:23:44 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:
Avoid double-read non volatile static field
This looks fine to me. I created the bug for this: https://bugs.openjdk.java.net/browse/JDK-8264032 -- please change the PR synopsis to "8264032: Improve thread safety of Runtime.version()". ------------- Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 13:23:44 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:
Avoid double-read non volatile static field
This shouldn't be a problem. Version is a pojo and value based, and the computed result may be different instances but would return `true` for `equals` and should have no problems src/java.base/share/classes/java/lang/Runtime.java line 824:
822: VersionProps.pre(), VersionProps.build(), 823: VersionProps.optional()); 824: version = v;
Can't this just be `return version = new Version(...` than reassigning to a local variable for no good? ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 16:13:21 GMT, liach <github.com+7806504+liach@openjdk.org> wrote:
This shouldn't be a problem.
What do you mean by "this"? Double racy read? There are 2 separate reads of fields. First can return non-null value, while second still can get `null`
src/java.base/share/classes/java/lang/Runtime.java line 824:
822: VersionProps.pre(), VersionProps.build(), 823: VersionProps.optional()); 824: version = v;
Can't this just be `return version = new Version(...` than reassigning to a local variable for no good?
It's matter of style. I've seen both styles are acceptable in JDK codebase. I personally prefer placing assigning each variable into separate lines. ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 16:46:06 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:
There are 2 separate reads of fields. First can return non-null value, while second still can get null
Can this really happen? If this `version` field has been updated, its value is definitely no longer `null`. And before the second field read the field is always set to a non-null value on the current thread. I fail to understand why the second read can still yield a null given the fact that the current thread has updated it to a non-null value and other threads may have updated it to a non-null value.
src/java.base/share/classes/java/lang/Runtime.java line 824:
822: VersionProps.pre(), VersionProps.build(), 823: VersionProps.optional()); 824: version = v;
Can't this just be `return version = new Version(...` than reassigning to a local variable for no good?
It's matter of style. I've seen both styles are acceptable in JDK codebase. I personally prefer placing assigning each variable into separate lines.
Doesn't using `return version = new Version(...` allow one local variable to be effectively final and save two aload/astore opcodes? Guess jvm optimizes it ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 16:59:45 GMT, liach <github.com+7806504+liach@openjdk.org> wrote:
This shouldn't be a problem.
What do you mean by "this"? Double racy read? There are 2 separate reads of fields. First can return non-null value, while second still can get `null`
There are 2 separate reads of fields. First can return non-null value, while second still can get null
Can this really happen? If this `version` field has been updated, its value is definitely no longer `null`. And before the second field read the field is always set to a non-null value on the current thread. I fail to understand why the second read can still yield a null given the fact that the current thread has updated it to a non-null value and other threads may have updated it to a non-null value.
I'm not really a JMM mastermind, but I know that @shipilev knows a thing or two about it :) See https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-... ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 13:23:44 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:
Avoid double-read non volatile static field
Marked as reviewed by alanb (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
On Tue, 23 Feb 2021 13:23:44 GMT, Andrey Turbanov <github.com+741251+turbanoff@openjdk.org> wrote:
Avoid double-read non volatile static field
This pull request has now been integrated. Changeset: 23353626 Author: Andrey Turbanov <turbanoff@gmail.com> Committer: Aleksey Shipilev <shade@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/23353626 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod 8264032: Improve thread safety of Runtime.version() Reviewed-by: shade, alanb ------------- PR: https://git.openjdk.java.net/jdk/pull/2691
participants (4)
-
Alan Bateman
-
Aleksey Shipilev
-
Andrey Turbanov
-
liach