Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v2]
On Mon, 6 Jun 2022 17:31:15 GMT, Naoto Sato wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8287860: Mark hash fields with @Stable > > src/java.base/share/classes/java/util/Locale.java line 1084: > >> 1082: Properties props = GetPropertyAction.privilegedGetProperties(); >> 1083: >> 1084: Locale defaultLocale = Locale.defaultLocale; > > I'd use a different local variable name so that it won't clash with the field > name. The same holds for other locations (`isoLanguages` and `languageTag`) Done! - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v2]
On Mon, 6 Jun 2022 14:36:03 GMT, Сергей Цыпанов wrote: >> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be >> volatile, even in case of race in the worst case it is recalculated at most >> once per thread >> - `defaultLocale` field is read multiple times in `initDefault()` >> - `isoLanguages` is accessed multiple times in `getISOLanguages()` >> - `languageTag` is read twice in `toLanguageTag()` > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8287860: Mark hash fields with @Stable src/java.base/share/classes/java/util/Locale.java line 1084: > 1082: Properties props = GetPropertyAction.privilegedGetProperties(); > 1083: > 1084: Locale defaultLocale = Locale.defaultLocale; I'd use a different local variable name so that it won't clash with the field name. The same holds for other locations (`isoLanguages` and `languageTag`) - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v3]
On Mon, 6 Jun 2022 20:19:22 GMT, Сергей Цыпанов wrote: >> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be >> volatile, even in case of race in the worst case it is recalculated at most >> once per thread >> - `defaultLocale` field is read multiple times in `initDefault()` >> - `isoLanguages` is accessed multiple times in `getISOLanguages()` >> - `languageTag` is read twice in `toLanguageTag()` > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8287860: Rename local vars Looks good. Thanks for fixing this. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v3]
> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be > volatile, even in case of race in the worst case it is recalculated at most > once per thread > - `defaultLocale` field is read multiple times in `initDefault()` > - `isoLanguages` is accessed multiple times in `getISOLanguages()` > - `languageTag` is read twice in `toLanguageTag()` Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8287860: Rename local vars - Changes: - all: https://git.openjdk.java.net/jdk/pull/9041/files - new: https://git.openjdk.java.net/jdk/pull/9041/files/d4534346..454621d4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9041=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9041=01-02 Stats: 14 lines in 1 file changed: 0 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/9041.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9041/head:pull/9041 PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v2]
On Mon, 6 Jun 2022 13:34:27 GMT, liach wrote: >> These fields can only be written once besides the default values, but they >> don't have to be written in the static initializer or constructor. So when a >> non-zero hash code is written, it's cached as if it's a final field. For a >> zero value, it would always undergo extra calculations like before and write >> multiple times, but the writes don't matter as it would only write zero, >> which would not yield false hash code if a previously written 0 value is >> cached. > >> Shouldn't the fields annotated with `@Stable` be `final` as well? > > You might have seen `@Stable final` fields. Those are only meaningful for > arrays, where such caching happens at each array index. For non-arrays, > `@Stable final` is simply equivalent to `final`, and simple `@Stable` acts as > what I said above. Done! - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale [v2]
> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be > volatile, even in case of race in the worst case it is recalculated at most > once per thread > - `defaultLocale` field is read multiple times in `initDefault()` > - `isoLanguages` is accessed multiple times in `getISOLanguages()` > - `languageTag` is read twice in `toLanguageTag()` Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8287860: Mark hash fields with @Stable - Changes: - all: https://git.openjdk.java.net/jdk/pull/9041/files - new: https://git.openjdk.java.net/jdk/pull/9041/files/d64917f1..d4534346 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9041=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9041=00-01 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/9041.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9041/head:pull/9041 PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale
On Mon, 6 Jun 2022 13:28:44 GMT, Сергей Цыпанов wrote: >> src/java.base/share/classes/java/util/Locale.java line 2260: >> >>> 2258: * Calculated hashcode >>> 2259: */ >>> 2260: private transient volatile int hashCodeValue; >> >> We can additionally annotate such cache fields with `@Stable` so as to >> enable constant-folding optimizations from the hotspot JIT. > > Shouldn't the fields annotated with `@Stable` be `final` as well? These fields can only be written once besides the default values, but they don't have to be written in the static initializer or constructor. So when a non-zero hash code is written, it's cached as if it's a final field. For a zero value, it would always undergo extra calculations like before and write multiple times, but the writes don't matter as it would only write zero, which would not yield false hash code if a previously written 0 value is cached. - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale
On Mon, 6 Jun 2022 13:32:21 GMT, liach wrote: >> Shouldn't the fields annotated with `@Stable` be `final` as well? > > These fields can only be written once besides the default values, but they > don't have to be written in the static initializer or constructor. So when a > non-zero hash code is written, it's cached as if it's a final field. For a > zero value, it would always undergo extra calculations like before and write > multiple times, but the writes don't matter as it would only write zero, > which would not yield false hash code if a previously written 0 value is > cached. > Shouldn't the fields annotated with `@Stable` be `final` as well? You might have seen `@Stable final` fields. Those are only meaningful for arrays, where such caching happens at each array index. For non-arrays, `@Stable final` is simply equivalent to `final`, and simple `@Stable` acts as what I said above. - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale
On Mon, 6 Jun 2022 13:20:31 GMT, liach wrote: >> - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be >> volatile, even in case of race in the worst case it is recalculated at most >> once per thread >> - `defaultLocale` field is read multiple times in `initDefault()` >> - `isoLanguages` is accessed multiple times in `getISOLanguages()` >> - `languageTag` is read twice in `toLanguageTag()` > > src/java.base/share/classes/java/util/Locale.java line 2260: > >> 2258: * Calculated hashcode >> 2259: */ >> 2260: private transient volatile int hashCodeValue; > > We can additionally annotate such cache fields with `@Stable` so as to enable > constant-folding optimizations from the hotspot JIT. Shouldn't the fields annotated with `@Stable` be `final` as well? - PR: https://git.openjdk.java.net/jdk/pull/9041
Re: RFR: 8287860: Revise usage of volatile in j.u.Locale
On Mon, 6 Jun 2022 12:58:39 GMT, Сергей Цыпанов wrote: > - cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be > volatile, even in case of race in the worst case it is recalculated at most > once per thread > - `defaultLocale` field is read multiple times in `initDefault()` > - `isoLanguages` is accessed multiple times in `getISOLanguages()` > - `languageTag` is read twice in `toLanguageTag()` src/java.base/share/classes/java/util/Locale.java line 2260: > 2258: * Calculated hashcode > 2259: */ > 2260: private transient volatile int hashCodeValue; We can additionally annotate such cache fields with `@Stable` so as to enable constant-folding optimizations from the hotspot JIT. - PR: https://git.openjdk.java.net/jdk/pull/9041
RFR: 8287860: Revise usage of volatile in j.u.Locale
- cached hash code of `Locale` and `Locale$LanguageRange` shouldn't be volatile, even in case of race in the worst case it is recalculated at most once per thread - `defaultLocale` field is read multiple times in `initDefault()` - `isoLanguages` is accessed multiple times in `getISOLanguages()` - `languageTag` is read twice in `toLanguageTag()` - Commit messages: - 8287860: Revise usage of volatile in j.u.Locale Changes: https://git.openjdk.java.net/jdk/pull/9041/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9041=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287860 Stats: 12 lines in 2 files changed: 3 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/9041.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9041/head:pull/9041 PR: https://git.openjdk.java.net/jdk/pull/9041
