Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote: >> This patch refactors Locale.getDefault(Category) so that the volatile field >> holding the Locale is typically only read once. This has a small performance >> advantage, and might be more robust if initialization is racy. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix omitted synchronized Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
No, I accidentally posted numbers for an apples to oranges comparison (-t 1 vs
-t 4 on the same system). The final version does not differ in performance from
this version when comparing like for like.
Hämta Outlook för Android<https://aka.ms/ghei36>
From: core-libs-dev on behalf of Sergey
Bylokhov
Sent: Saturday, March 6, 2021 9:39:07 PM
To: core-libs-dev@openjdk.java.net ;
i18n-...@openjdk.java.net
Subject: Re: RFR: 8263090: Avoid reading volatile fields twice in
Locale.getDefault(Category) [v2]
On Sat, 6 Mar 2021 13:34:14 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/util/Locale.java line 946:
>>
>>> 944: Locale loc = defaultDisplayLocale; // volatile read
>>> 945: if (loc == null) {
>>> 946: loc = getDisplayLocale();
>>
>> Just interesting how did you check that the performance difference is
>> because of volatile read, and not because of replacing of the switch by the
>> "if"?
>
> I started out with this variant, only removing the double volatile reads:
>
> public static Locale getDefault(Locale.Category category) {
> // do not synchronize this method - see 4071298
> Locale loc;
> switch (category) {
> case DISPLAY:
> loc = defaultDisplayLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultDisplayLocale;
> if (loc == null) {
> loc = defaultDisplayLocale = initDefault(category);
> }
> }
> }
> return loc;
> case FORMAT:
> loc = defaultFormatLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultFormatLocale;
> if (loc == null) {
> loc = defaultFormatLocale = initDefault(category);
> }
> }
> }
> return loc;
> default:
> assert false: "Unknown Category";
> }
> return getDefault();
> }
>
> Scores were the same:
> Benchmark Mode Cnt Score Error Units
> LocaleDefaults.getDefault avgt5 10.045 ± 0.032 ns/op
> LocaleDefaults.getDefaultDisplay avgt5 11.301 ± 0.053 ns/op
> LocaleDefaults.getDefaultFormat avgt5 11.303 ± 0.054 ns/op
>
> I then refactored and checked that the refactorings were performance neutral.
And it is faster than the final version?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Sat, 6 Mar 2021 13:34:14 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/util/Locale.java line 946:
>>
>>> 944: Locale loc = defaultDisplayLocale; // volatile read
>>> 945: if (loc == null) {
>>> 946: loc = getDisplayLocale();
>>
>> Just interesting how did you check that the performance difference is
>> because of volatile read, and not because of replacing of the switch by the
>> "if"?
>
> I started out with this variant, only removing the double volatile reads:
>
> public static Locale getDefault(Locale.Category category) {
> // do not synchronize this method - see 4071298
> Locale loc;
> switch (category) {
> case DISPLAY:
> loc = defaultDisplayLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultDisplayLocale;
> if (loc == null) {
> loc = defaultDisplayLocale = initDefault(category);
> }
> }
> }
> return loc;
> case FORMAT:
> loc = defaultFormatLocale;
> if (loc == null) {
> synchronized(Locale.class) {
> loc = defaultFormatLocale;
> if (loc == null) {
> loc = defaultFormatLocale = initDefault(category);
> }
> }
> }
> return loc;
> default:
> assert false: "Unknown Category";
> }
> return getDefault();
> }
>
> Scores were the same:
> Benchmark Mode Cnt Score Error Units
> LocaleDefaults.getDefault avgt5 10.045 ± 0.032 ns/op
> LocaleDefaults.getDefaultDisplay avgt5 11.301 ± 0.053 ns/op
> LocaleDefaults.getDefaultFormat avgt5 11.303 ± 0.054 ns/op
>
> I then refactored and checked that the refactorings were performance neutral.
And it is faster than the final version?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Sat, 6 Mar 2021 05:51:17 GMT, Sergey Bylokhov wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix omitted synchronized
>
> src/java.base/share/classes/java/util/Locale.java line 946:
>
>> 944: Locale loc = defaultDisplayLocale; // volatile read
>> 945: if (loc == null) {
>> 946: loc = getDisplayLocale();
>
> Just interesting how did you check that the performance difference is because
> of volatile read, and not because of replacing of the switch by the "if"?
I started out with this variant, only removing the double volatile reads:
public static Locale getDefault(Locale.Category category) {
// do not synchronize this method - see 4071298
Locale loc;
switch (category) {
case DISPLAY:
loc = defaultDisplayLocale;
if (loc == null) {
synchronized(Locale.class) {
loc = defaultDisplayLocale;
if (loc == null) {
loc = defaultDisplayLocale = initDefault(category);
}
}
}
return loc;
case FORMAT:
loc = defaultFormatLocale;
if (loc == null) {
synchronized(Locale.class) {
loc = defaultFormatLocale;
if (loc == null) {
loc = defaultFormatLocale = initDefault(category);
}
}
}
return loc;
default:
assert false: "Unknown Category";
}
return getDefault();
}
Scores were the same:
Benchmark Mode Cnt Score Error Units
LocaleDefaults.getDefault avgt5 10.045 ± 0.032 ns/op
LocaleDefaults.getDefaultDisplay avgt5 11.301 ± 0.053 ns/op
LocaleDefaults.getDefaultFormat avgt5 11.303 ± 0.054 ns/op
I then refactored and checked that the refactorings were performance neutral.
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Sat, 6 Mar 2021 12:36:02 GMT, David Holmes wrote: > If I read the order right your benchmark findings were done before you added > the missing synchronized - correct? > > AFAICS the only unnecessary volatile read is on the return statement and you > could fix that without doing the other refactoring. I don't see how > introducing an extra method call can aid performance. Note that the score for the Display and the Format case were identical, even though one was missing synchronized. I've re-run the benchmarks after the fix and the same applies. The methods will only ever be called during initialization, usually only once. Extracting them to separate methods helps outline initialization code from the hot path. Such outlining is a standard technique to help the JITs do the right thing, for example by ensuring you don't stumble on things like inlining thresholds. - PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:58:44 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix omitted synchronized > > Looks good. If I read the order right your benchmark findings were done before you added the missing synchronized - correct? AFAICS the only unnecessary volatile read is on the return statement and you could fix that without doing the other refactoring. I don't see how introducing an extra method call can aid performance. - PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote:
>> This patch refactors Locale.getDefault(Category) so that the volatile field
>> holding the Locale is typically only read once. This has a small performance
>> advantage, and might be more robust if initialization is racy.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix omitted synchronized
src/java.base/share/classes/java/util/Locale.java line 946:
> 944: Locale loc = defaultDisplayLocale; // volatile read
> 945: if (loc == null) {
> 946: loc = getDisplayLocale();
Just interesting how did you check that the performance difference is because
of volatile read, and not because of replacing of the switch by the "if"?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 18:53:29 GMT, Claes Redestad wrote: >> This patch refactors Locale.getDefault(Category) so that the volatile field >> holding the Locale is typically only read once. This has a small performance >> advantage, and might be more robust if initialization is racy. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Fix omitted synchronized Looks good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
> This patch refactors Locale.getDefault(Category) so that the volatile field > holding the Locale is typically only read once. This has a small performance > advantage, and might be more robust if initialization is racy. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Fix omitted synchronized - Changes: - all: https://git.openjdk.java.net/jdk/pull/2845/files - new: https://git.openjdk.java.net/jdk/pull/2845/files/5d2f0da4..4980d2d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2845=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2845=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2845.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2845/head:pull/2845 PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) [v2]
On Fri, 5 Mar 2021 17:29:16 GMT, Naoto Sato wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix omitted synchronized
>
> src/java.base/share/classes/java/util/Locale.java line 959:
>
>> 957: }
>> 958:
>> 959: private static Locale getDisplayLocale() {
>
> Should this be `synchronized`?
Good catch! Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)
On Fri, 5 Mar 2021 14:14:14 GMT, Claes Redestad wrote:
> This patch refactors Locale.getDefault(Category) so that the volatile field
> holding the Locale is typically only read once. This has a small performance
> advantage, and might be more robust if initialization is racy.
src/java.base/share/classes/java/util/Locale.java line 959:
> 957: }
> 958:
> 959: private static Locale getDisplayLocale() {
Should this be `synchronized`?
-
PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)
On Fri, 5 Mar 2021 14:14:14 GMT, Claes Redestad wrote: > This patch refactors Locale.getDefault(Category) so that the volatile field > holding the Locale is typically only read once. This has a small performance > advantage, and might be more robust if initialization is racy. Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2845
Re: RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)
On Fri, 5 Mar 2021 14:14:14 GMT, Claes Redestad wrote: > This patch refactors Locale.getDefault(Category) so that the volatile field > holding the Locale is typically only read once. This has a small performance > advantage, and might be more robust if initialization is racy. Results on the provided, simple microbenchmark Baseline: Benchmark Mode Cnt Score Error Units LocaleDefaults.getDefault avgt5 11.142 ± 0.084 ns/op LocaleDefaults.getDefaultDisplay avgt5 14.024 ± 0.063 ns/op LocaleDefaults.getDefaultFormat avgt5 14.001 ± 0.072 ns/op Patch: Benchmark Mode Cnt Score Error Units LocaleDefaults.getDefault avgt5 11.210 ± 0.057 ns/op LocaleDefaults.getDefaultDisplay avgt5 12.597 ± 0.022 ns/op LocaleDefaults.getDefaultFormat avgt5 12.608 ± 0.049 ns/op - PR: https://git.openjdk.java.net/jdk/pull/2845
RFR: 8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)
This patch refactors Locale.getDefault(Category) so that the volatile field holding the Locale is typically only read once. This has a small performance advantage, and might be more robust if initialization is racy. - Commit messages: - Add microbenchmark - Reduce volatile reads in Locale.getDefault(Category) Changes: https://git.openjdk.java.net/jdk/pull/2845/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2845=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263090 Stats: 92 lines in 2 files changed: 72 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/2845.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2845/head:pull/2845 PR: https://git.openjdk.java.net/jdk/pull/2845
