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

8290079: Reduce interaction with volatile in static initializer of BigInteger #9451

Closed
wants to merge 2 commits into from

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Jul 11, 2022

BigInteger.powerCache is volatile and should be assigned only once in static initializer.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8290079: Reduce interaction with volatile in static initializer of BigInteger

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9451/head:pull/9451
$ git checkout pull/9451

Update a local copy of the PR:
$ git checkout pull/9451
$ git pull https://git.openjdk.org/jdk pull/9451/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9451

View PR using the GUI difftool:
$ git pr show -t 9451

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9451.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 11, 2022

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 11, 2022
@openjdk
Copy link

openjdk bot commented Jul 11, 2022

@stsypanov The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jul 11, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 11, 2022

Webrevs

Copy link
Member

@jaikiran jaikiran left a comment

Choose a reason for hiding this comment

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

The change looks fine to me. Please wait for at least another review from someone more familiar with this area, before merging.

@openjdk
Copy link

openjdk bot commented Jul 12, 2022

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8290079
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

openjdk bot commented Jul 12, 2022

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

8290079: Reduce interaction with volatile in static initializer of BigInteger

Reviewed-by: jpai, rriggs, darcy

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

  • 2583feb: 8290023: Remove use of IgnoreUnrecognizedVMOptions in gc tests
  • 44fb92e: 8290197: test/jdk/java/nio/file/Files/probeContentType/Basic.java fails on some systems for the ".rar" extension
  • f528124: 8289284: jdk.tracePinnedThreads output confusing when pinned due to native frame
  • 572c14e: 8288624: Cleanup CommentHelper.getText0
  • 6e18883: 8290162: Reset recursion counter missed in fix of JDK-8224267
  • 31f7fc0: 8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field
  • d9ca438: Merge
  • 3164c98: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM
  • c3806b9: 8289709: fatal error: stuck in JvmtiVTMSTransitionDisabler::disable_VTMS_transitions
  • 39715f3: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
  • ... and 34 more: https://git.openjdk.org/jdk/compare/46251bc6e248a19e8d78173ff8d0502c68ee1acb...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jaikiran, @RogerRiggs, @jddarcy) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 12, 2022
@rgiulietti
Copy link
Contributor

(Not a reviewer) While the change doesn't hurt, I doubt that access to a (static) volatile in a static initializer is ever contended.

@stsypanov
Copy link
Contributor Author

@rgiulietti AFAIK volatile access is more expensive than plain one regardless contention

@rgiulietti
Copy link
Contributor

Usually yes, but since a static initializer is executed by at most one thread by using a locking protocol before any other static code is ever executed, the runtime could (but I'm not sure it it really does) treat the volatile in the for loop as a local.
But I would approve your change because it makes this more explicit.

@stsypanov
Copy link
Contributor Author

@rgiulietti I've copy-pasted class-loading benchmark from JMH samples

@State(Scope.Thread)
@Warmup(iterations = 10, time = 1)
@Measurement(iterations = 10, time = 5)
@Fork(value = 5, jvmArgsAppend = {"-Xms1g", "-Xmx1g"})
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class Classy {

    @Benchmark
    public Class<?> load() throws ClassNotFoundException {
        return Class.forName("com.tsypanov.slp.Sample", true, new XLoader());
    }

    public static class XLoader extends URLClassLoader {

        private static final byte[] X_BYTECODE = new byte[]{ /*..*/};

        public XLoader() {
            super(new URL[0], ClassLoader.getSystemClassLoader());
        }

        @Override
        protected Class<?> findClass(final String name) {
            return defineClass(name, X_BYTECODE, 0, X_BYTECODE.length);
        }
    }
}

and used it to measure loading a class from byte[] copied from the following class compiled:

class Sample {
  static volatile int[] items = new int[100];
  static {
    for (int i = 0; i < items.length; i++) {
      items[i] = ThreadLocalRandom.current().nextInt();
    }
  }
}

I ran the benchmark with java -jar target/sleep-benchmarks.jar Classy -prof cl and for the version above got these results (Java 17):

Benchmark                       Mode  Cnt       Score      Error        Units
Classy.load                     avgt   50   96247.202 ±  548.137        ns/op
Classy.load:·class.load         avgt   50  259912.089 ± 1485.314  classes/sec
Classy.load:·class.load.norm    avgt   50       1.000 ±    0.001   classes/op
Classy.load:·class.unload       avgt   50  260243.318 ± 3673.515  classes/sec
Classy.load:·class.unload.norm  avgt   50       1.001 ±    0.014   classes/op

Then I've modified the class in the same way I did in this PR:

class Sample {
  static volatile int[] items;
  static {
    int[] items = new int[100];
    for (int i = 0; i < items.length; i++) {
      items[i] = ThreadLocalRandom.current().nextInt();
    }
    Sample.items = items;
  }
}

and for modified code got


Benchmark                       Mode  Cnt       Score      Error        Units
Classy.load                     avgt   50   63955.673 ±  147.470        ns/op
Classy.load:·class.load         avgt   50  391101.854 ±  925.013  classes/sec
Classy.load:·class.load.norm    avgt   50       1.000 ±    0.001   classes/op
Classy.load:·class.unload       avgt   50  390800.851 ± 2307.589  classes/sec
Classy.load:·class.unload.norm  avgt   50       0.999 ±    0.006   classes/op

From this I conclude that volatile costs are still there no matter whether we deal with static or non-static initializers.

@rgiulietti
Copy link
Contributor

@stsypanov Hi Сергей, thanks for the convincing measurements!

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jddarcy jddarcy left a comment

Choose a reason for hiding this comment

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

Version with renamed local variable looks fine.

@stsypanov
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 13, 2022
@openjdk
Copy link

openjdk bot commented Jul 13, 2022

@stsypanov
Your change (at version 1f51992) is now ready to be sponsored by a Committer.

@jddarcy
Copy link
Member

jddarcy commented Jul 13, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jul 13, 2022

Going to push as commit c83fcbd.
Since your change was applied there have been 47 commits pushed to the master branch:

  • 74ac5df: 8290234: [JVMCI] use JVMCIKlassHandle to protect raw Klass* values from concurrent G1 scanning
  • 5e3ecff: 8290253: gc/g1/TestVerificationInConcurrentCycle.java#id1 fails with "Error. can't find sun.hotspot.WhiteBox in test directory or libraries"
  • 5358045: 8066859: java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: Reference Handler thread died
  • 2583feb: 8290023: Remove use of IgnoreUnrecognizedVMOptions in gc tests
  • 44fb92e: 8290197: test/jdk/java/nio/file/Files/probeContentType/Basic.java fails on some systems for the ".rar" extension
  • f528124: 8289284: jdk.tracePinnedThreads output confusing when pinned due to native frame
  • 572c14e: 8288624: Cleanup CommentHelper.getText0
  • 6e18883: 8290162: Reset recursion counter missed in fix of JDK-8224267
  • 31f7fc0: 8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field
  • d9ca438: Merge
  • ... and 37 more: https://git.openjdk.org/jdk/compare/46251bc6e248a19e8d78173ff8d0502c68ee1acb...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 13, 2022
@openjdk openjdk bot closed this Jul 13, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 13, 2022
@openjdk
Copy link

openjdk bot commented Jul 13, 2022

@jddarcy @stsypanov Pushed as commit c83fcbd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@stsypanov stsypanov deleted the 8290079 branch July 13, 2022 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants