8254146: Avoid unnecessary volatile write on new AtomicBoolean(false)

[dreis2211]

Hi,

the following PR optimizes new AtomicBoolean(boolean) by avoiding the volatile write in case false is passed. Essentially, it changes the ternary operator to a simple if without the else that would cause the volatile write.

The resulting bytecode seems to also benefit from the change:

    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: aload_0
       5: iload_1
       6: ifeq          13
       9: iconst_1
      10: goto          14
      13: iconst_0
      14: putfield      #7                  // Field value:I
      17: return

After:

    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: iload_1
       5: ifeq          13
       8: aload_0
       9: iconst_1
      10: putfield      #7                  // Field value:I
      13: return

A simple benchmark that returns new AtomicBoolean(false) shows the following results, that brings it on par to new AtomicBoolean():

MyBenchmark.empty                                         avgt   10     3,103 ±   0,246   ns/op
MyBenchmark.explicitNew                                   avgt   10     2,966 ±   0,071   ns/op
MyBenchmark.explicitOld                                   avgt   10     7,738 ±   0,321   ns/op

In case you think this is worthwhile I'd be happy if this is sponsored.
Cheers,
Christoph

---

Progress

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

Issue

• JDK-8254146: Avoid unnecessary volatile write on new AtomicBoolean(false)

Reviewers

• Claes Redestad (@cl4es - Reviewer)
• Roger Riggs (@RogerRiggs - Reviewer)
• Chris Hegarty (@ChrisHegarty - Reviewer)

Download

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

[bridgekeeper]

Hi @dreis2211, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dreis2211" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

@dreis2211 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.

[dreis2211]

I have contributed before the move to GitHub and signed the OCA there. Can anybody tell me what the process is for this case?

[jerboaa]

@dreis2211 Have you tried what the bot suggested?

Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

[dreis2211]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[dreis2211]

@jerboaa Since I have signed the OCA before the move to GitHub there is no connection to my GitHub handle, so I was confused by that if that means I have to completely sign the OCA again.

[mlbridge]

Webrevs

• 00: Full (a98247a)

[openjdk]

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

8254146: Avoid unnecessary volatile write on new AtomicBoolean(false)

Reviewed-by: redestad, rriggs, chegar

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

• 894ec76: 8254027: gc/g1/TestHumongousConcurrentStartUndo.java failed with "'Concurrent Mark Cycle' missing from stdout/stderr"
• bc23690: 8254173: Add Zero, Minimal hotspot targets to submit workflow
• e1187c4: 8248262: Wrong link target in ModuleDescriptor#isAutomatic's API documentation
• 9cdfd0f: 8254096: remove jdk.test.lib.Utils::getMandatoryProperty(String) method
• d1e94ee: 8253909: Implement detailed map file for CDS
• 7733a0e: 8254182: remove Utils.tryFindJvmPid/waitForJvmPid
• 4e5ef30: 8254104: MethodCounters must exist before nmethod is installed
• fd0cb98: 8253901: ARM32: SIGSEGV during monitorexit due to incorrect register use (after JDK-8253540)
• abe2593: 8232092: (fs) Files::isWritable returns false on a writeable root directory (win)
• 5a9bd41: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests
• ... and 35 more: https://git.openjdk.java.net/jdk/compare/88d75c9ad536282ea9b6860379aff76c7d516879...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 (@cl4es, @RogerRiggs, @ChrisHegarty) 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).

[dreis2211]

/integrate

[AlanBateman]

I assume this should be coordinated with an update to the jsr166 CVS too.

[openjdk]

@dreis2211
Your change (at version a98247a) is now ready to be sponsored by a Committer.

[dreis2211]

@AlanBateman What would be the label (or workflow) for that now?

[cl4es]

@AlanBateman What would be the label (or workflow) for that now?

@dreis2211 what Alan reminded us of here is that all of j.u.concurrent is maintained upstream from the OpenJDK, and that this change needs to be coordinated with the maintainers there. Either by getting an OK to go ahead and push this as-is, or by contributing this patch to the JSR166 repo directly (which will then be integrated back into the OpenJDK at some point). See http://gee.cs.oswego.edu/dl/concurrency-interest/

This isn't anything new since the GitHub migration: Various bits and pieces of the OpenJDK sources are maintained upstream, and it's not always easy to igure out exactly where or how to go about upstreaming a change.

[dreis2211]

@cl4es Thanks. My question was more referring to how the PR tooling should be used these days to notify the maintainers. As JSR166 is a crucial piece of the JDK I was wondering if there might be a way already via a label or something.
Or if this needs some good old mail (to concurrency-interest@cs.oswego.edu I suppose?)

[shipilev]

You can probably go ahead and ping @DougLea (like this).

[DougLea]

I'm a little hesitant about the precedent of checking for 0 in every ctor argument for a volatile field.
An alternative here worth measuring is: VALUE.setRelease(this, initialValue ? 1 : 0);

[dreis2211]

I'm a little hesitant about the precedent of checking for 0 in every ctor argument for a volatile field.

I'm confused by this if I'm entirely honest @DougLea . Where do we check for 0 in the proposed change?

[DougLea]

As in: If unnecessarily writing 0 to a voltaile field in a ctor is expensive enough for a code work-around here, don't you think it would be better to teach a compiler to avoid it?

[dreis2211]

I see what you mean now - thanks! Given that there exists already an empty AtomicBoolean constructor that avoids the volatile write, I didn't see that as a big problem to be honest. Also: The proposed "workaround" doesn't seem to be overly complicated - personally I find the empty constructor to be the bigger workaround and less explicit.

The same argument applies to initializing volatile fields with their default value, if I'm thinking about it longer. Of course one would wish that the compiler would be able to avoid it, but there have been examples in the past already where the explicit volatile write was avoided (e.g. in JDK-8145680 instead of working on JDK-8145948 for example).

Overall, I think it's a reasonably simple improvement with a good performance gain for relatively little cost, but after all I'm in your hands when it comes to approving the proposed change.

[DougLea]

OK. I won't resist doing this as a one-shot. We'll include in jsr166 sources, so it won't matter if done now or later along with integration pass. (Well, except the braces, which are stylistically not used in j.u.c. code)

[RogerRiggs]

/sponsor

[openjdk]

@RogerRiggs @dreis2211 Since your change was applied there have been 54 commits pushed to the master branch:

• 6d13c76: 8253191: C2: Masked byte comparisons with large masks produce wrong result on x86
• a191c58: 8253404: C2: assert(C->live_nodes() <= C->max_node_limit()) failed: Live Node limit exceeded limit
• 6bc4931: 8253717: Relocate stack overflow code out of thread.hpp/cpp
• 782d45b: 8242882: opening jar file with large manifest might throw NegativeArraySizeException
• f860372: 8253566: clazz.isAssignableFrom will return false for interface implementors
• 66f27b5: 8254015: copy_to_survivor_space should use in-hand klass for scanning
• 76a5852: 8253756: C2 CompilerThread0 crash in Node::add_req(Node*)
• 8f9e479: 8254144: Non-x86 Zero builds fail with return-type warning in os_linux_zero.cpp
• 7952c06: 8254166: Zero: return-type warning in zeroInterpreter_zero.cpp
• 894ec76: 8254027: gc/g1/TestHumongousConcurrentStartUndo.java failed with "'Concurrent Mark Cycle' missing from stdout/stderr"
• ... and 44 more: https://git.openjdk.java.net/jdk/compare/88d75c9ad536282ea9b6860379aff76c7d516879...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7e82ba1.

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

[stsypanov]

I appologise for writing this to already closed issue, but Christoph's approach seems to be applicable for at least AtomicInteger and AtomicLong, because they are often explicitly zeroed at creation time (see one of my previous PRs spring-projects/spring-framework#25846) and there we have the same effect as in AtomicBoolean:

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(value = Mode.AverageTime)
public class AtomicBenchmark {
  @Benchmark
  public Object defaultValue() {
    return new AtomicInteger();
  }
  @Benchmark
  public Object explicitValue() {
    return new AtomicInteger(0);
  }
}

Semantically both new AtomicInteger() and new AtomicInteger(0) are the same, but explicitValue() is much slower:

Benchmark                      Mode  Cnt   Score   Error  Units
AtomicBenchmark.defaultValue   avgt   30   4.778 ± 0.403  ns/op
AtomicBenchmark.explicitValue  avgt   30  11.846 ± 0.273  ns/op

So the same pattern as we used here could be applied for AtomicInteger:

public AtomicInteger(int initialValue) {
  if (initialValue != 0 {
    value = initialValue;
  }
}

What do you think?

[cl4es]

What do you think?

For AtomicBoolean, this optimization piggy-backed on an existing test, just rewriting it so that no action is taken when none is needed. So it's never a pessimization, and even improves bytecode size marginally. So it'll be a marginal gain even if the current overhead of writing to a volatile field in a constructor is removed/reduced.

For AtomicInteger/-Long, we'd have to add a test that's not there, so we'd pessimize non-zero cases by adding some bytecode and a branch to remove the overhead. An overhead that can already be avoided by calling the empty constructor instead. So I think it's questionable to do this.

In the short term we could consider a point fix to replace existing use of new AtomicInteger/-Long(0) with new AtomicInteger/-Long(), but in the long term we should really try and optimize our JITs so that writing to a volatile field in constructors can be made cheaper.

[dreis2211]

@stsypanov What Claes said: I explicitly only did it for AtomicBoolean as the conditional expression was already present.

I don't think we should do it for the other Atomic classes for the mentioned reasons. Let's rather work on the JIT compiler for future optimizations in this area. The AtomicBoolean was really just an exception.

[stsypanov]

@cl4es , @dreis2211 thanks for explanation!

[mlbridge]

Mailing list message from Andrew Haley on core-libs-dev:

On 15/10/2020 11:41, Claes Redestad wrote:

In the short term we could consider a point fix to replace existing
use of `new AtomicInteger/-Long(0)` with `new
AtomicInteger/-Long()`, but in the long term we should really try
and optimize our JITs so that writing to a volatile field in
constructors can be made cheaper.

I guess it all depends on whether there's a happens-before
relationship between the volatile store to Field X in a constructor
and any subsequent get() of Field X in another thread. Intuitively I
would have thought so, but I can't find any guarantee in the spec.
Nonetheless, I think we'd have little gain by changing this and it
might break (arguably incorrect) programs.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[cl4es]

On 15/10/2020 11:41, Claes Redestad wrote:

In the short term we could consider a point fix to replace existing
use of new AtomicInteger/-Long(0) with new AtomicInteger/-Long(), but in the long term we should really try
and optimize our JITs so that writing to a volatile field in
constructors can be made cheaper.

I guess it all depends on whether there's a happens-before
relationship between the volatile store to Field X in a constructor
and any subsequent get() of Field X in another thread. Intuitively I
would have thought so, but I can't find any guarantee in the spec.
Nonetheless, I think we'd have little gain by changing this and it
might break (arguably incorrect) programs.

I misphrased somewhat: I think we should try and remove explicit stores of default values to volatile fields in constructors, as suggested by https://bugs.openjdk.java.net/browse/JDK-8145948

I can't think of a way this would break any program (even arguably incorrect ones), and would net us the gains shown in comments here without any point fixes.

[stsypanov]

@cl4es could I ask one more question? Would it be helpful to create a separate PR about removal of explicit zeroing of Atomic* classes (excluding AtomicBoolean), e.g. somethin like this master...stsypanov:atom-simpl ?

[cl4es]

@cl4es could I ask one more question? Would it be helpful to create a separate PR about removal of explicit zeroing of Atomic* classes (excluding AtomicBoolean), e.g. somethin like this master...stsypanov:atom-simpl ?

Sure. Fixing static finals is unlikely to matter much, but it's a trivial change and as long as it has a performance advantage it's good to be consistent.

General advice is to limit sweeping changes to related modules, and not include code maintained in some upstream repo. What you have here looks fine.

[stsypanov]

DOne #818