Skip to content
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

8263090: Avoid reading volatile fields twice in Locale.getDefault(Category) #2845

Closed
wants to merge 3 commits into from

Conversation

@cl4es
Copy link
Member

@cl4es cl4es commented Mar 5, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2845/head:pull/2845
$ git checkout pull/2845

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 5, 2021

👋 Welcome back redestad! 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.

@cl4es
Copy link
Member Author

@cl4es cl4es commented Mar 5, 2021

Results on the provided, simple microbenchmark

Baseline:

Benchmark                         Mode  Cnt   Score   Error  Units
LocaleDefaults.getDefault         avgt    5  11.142 ± 0.084  ns/op
LocaleDefaults.getDefaultDisplay  avgt    5  14.024 ± 0.063  ns/op
LocaleDefaults.getDefaultFormat   avgt    5  14.001 ± 0.072  ns/op

Patch:

Benchmark                         Mode  Cnt   Score   Error  Units
LocaleDefaults.getDefault         avgt    5  11.210 ± 0.057  ns/op
LocaleDefaults.getDefaultDisplay  avgt    5  12.597 ± 0.022  ns/op
LocaleDefaults.getDefaultFormat   avgt    5  12.608 ± 0.049  ns/op

@cl4es cl4es marked this pull request as ready for review Mar 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

@cl4es The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 5, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

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

8263090: Avoid reading volatile fields twice in Locale.getDefault(Category)

Reviewed-by: rriggs, naoto, serb

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

  • 3844ce4: 8261247: some compiler/whitebox/ tests fail w/ DeoptimizeALot
  • f2d0152: 8263043: Add test to verify order of tag output
  • 7182985: 8263104: fix warnings for empty paragraphs
  • 5eb2091: 8261689: javax/swing/JComponent/7154030/bug7154030.java still fails with "Exception: Failed to hide opaque button"
  • 75a5be8: 8263054: [testbug] SharedArchiveConsistency.java reuses jsa files
  • 2afbd5d: 8250804: Can't set the application icon image for Unity WM on Linux.
  • fa43f92: 8261845: File permissions of packages built by jpackage
  • 23ee60d: 8261008: Optimize Xor
  • e1cad97: 8262862: Harden tests sun/security/x509/URICertStore/ExtensionsWithLDAP.java and krb5/canonicalize/Test.java
  • 2c0507e: 8261812: C2 compilation fails with assert(!had_error) failed: bad dominance
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/d2c4ed08a2f78c22e4d59b6c29d29abf3202199d...master

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 openjdk bot added the ready label Mar 5, 2021
naotoj
naotoj approved these changes Mar 5, 2021
Copy link
Member

@naotoj naotoj left a comment

Looks good.

if (category == Category.DISPLAY) {
Locale loc = defaultDisplayLocale; // volatile read
if (loc == null) {
loc = getDisplayLocale();
Copy link
Member

@mrserb mrserb Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member Author

@cl4es cl4es Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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         avgt    5  10.045 ± 0.032  ns/op
LocaleDefaults.getDefaultDisplay  avgt    5  11.301 ± 0.053  ns/op
LocaleDefaults.getDefaultFormat   avgt    5  11.303 ± 0.054  ns/op

I then refactored and checked that the refactorings were performance neutral.

Copy link
Member

@mrserb mrserb Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it is faster than the final version?

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Mar 6, 2021

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.

@cl4es
Copy link
Member Author

@cl4es cl4es commented Mar 6, 2021

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 6, 2021

Mailing list message from Claes Redestad on i18n-dev:

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 <core-libs-dev-retn at openjdk.java.net> on behalf of Sergey Bylokhov <serb at openjdk.java.net>
Sent: Saturday, March 6, 2021 9:39:07 PM
To: core-libs-dev at openjdk.java.net <core-libs-dev at openjdk.java.net>; i18n-dev at openjdk.java.net <i18n-dev at 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 <redestad at openjdk.org> 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 avgt 5 10.045 ? 0.032 ns/op
LocaleDefaults.getDefaultDisplay avgt 5 11.301 ? 0.053 ns/op
LocaleDefaults.getDefaultFormat avgt 5 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

mrserb
mrserb approved these changes Mar 6, 2021
@cl4es
Copy link
Member Author

@cl4es cl4es commented Mar 8, 2021

/integrate

@openjdk openjdk bot closed this Mar 8, 2021
@openjdk openjdk bot added integrated and removed ready labels Mar 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 8, 2021

Pushed as commit 13625be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants