8345668: ZoneOffset.ofTotalSeconds performance regression #22854

naotoj · 2024-12-20T19:55:06Z

The change made in JDK-8288723 seems innocuous, but it caused this performance regression. Partially reverting the change (ones that involve computeIfAbsent()) to the original. Provided a benchmark that iterates the call to ZoneOffset.ofTotalSeconds(0) 1,000 times, which improves the operation time from 3,946ns to 2,241ns.

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-8345668: ZoneOffset.ofTotalSeconds performance regression (Bug - P3)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Andrey Turbanov (@turbanoff - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22854

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-20T19:55:31Z

👋 Welcome back naoto! 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 · 2024-12-20T19:55:35Z

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

8345668: ZoneOffset.ofTotalSeconds performance regression

Reviewed-by: rriggs, aturbanov

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

a77ed30: 8336412: sun.net.www.MimeTable has a few unused methods
e769b53: 8346193: CrashGCForDumpingJavaThread do not trigger expected crash build with clang17
a87bc7e: 8345374: Ubsan: runtime error: division by zero

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 · 2024-12-20T19:56:02Z

@naotoj 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 · 2024-12-20T19:59:49Z

Webrevs

src/java.base/share/classes/java/time/ZoneOffset.java

Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>

wenshao · 2024-12-20T23:05:06Z

src/java.base/share/classes/java/time/ZoneOffset.java

-            return SECONDS_CACHE.computeIfAbsent(totalSeconds, totalSecs -> {
-                ZoneOffset result = new ZoneOffset(totalSecs);
+            Integer totalSecs = totalSeconds;
+            ZoneOffset result = SECONDS_CACHE.get(totalSecs);


Here, each call may allocate an Integer object. The maximum number of ZoneOffsets that need to be cached here is only 148. Using AtomicReferenceArray is better than AtomicConcurrentHashMap.

For example:

static final AtomicReferenceArray<ZoneOffset> MINUTES_15_CACHE = new AtomicReferenceArray<>(37 * 4); public static ZoneOffset ofTotalSeconds(int totalSeconds) { // ... int minutes15Rem = totalSeconds / (15 * SECONDS_PER_MINUTE); if (totalSeconds - minutes15Rem * 15 * SECONDS_PER_MINUTE == 0) { int cacheIndex = minutes15Rem + 18 * 4; ZoneOffset result = MINUTES_15_CACHE.get(cacheIndex); if (result == null) { result = new ZoneOffset(totalSeconds); if (!MINUTES_15_CACHE.compareAndSet(cacheIndex, null, result)) { result = MINUTES_15_CACHE.get(minutes15Rem); } } return result; } // ... }

Hi Shaojin,
Thanks for the suggestion, but I am not planning to improve the code more than backing out the offending fix at this time. (btw, cache size would be 149 as 18:00 and -18:00 are inclusive)

Can I submit a PR to make this improvement?

@wenshao I agree with your proposal. Also for this part:

ZoneOffset result = MINUTES_15_CACHE.get(cacheIndex); if (result == null) { result = new ZoneOffset(totalSeconds); if (!MINUTES_15_CACHE.compareAndSet(cacheIndex, null, result)) { result = MINUTES_15_CACHE.get(minutes15Rem); } }

I recommend a rewrite:

ZoneOffset result = MINUTES_15_CACHE.getPlain(cacheIndex); if (result == null) { result = new ZoneOffset(totalSeconds); ZoneOffset existing = MINUTES_15_CACHE.compareAndExchange(cacheIndex, null, result); return existing == null ? result : existing; }

The getPlain is safe because ZoneOffset is thread safe, so you can use the object when you can observe a ZoneOffset object reference. Also compareAndExchange avoids extra operations if we failed to racily set the computed ZoneOffset.

liach · 2024-12-21T00:54:34Z

test/micro/org/openjdk/bench/java/time/ZoneOffsetBench.java

+    @Benchmark
+    public void ofTotalSeconds() {
+        for (int i = 0; i < 1_000; i++) {
+            ZoneOffset.ofTotalSeconds(0);


This benchmark method should accept a Blackhole, and the return value of ofTotalSeconds must be sent to the Blackhole.consume method.

This benchmark currently works probably because the cache interactions in ofTotalSeconds, which means JIT compilation cannot prove it is side-effect free. Had it been as simple as a decimal computation or if the cache becomes a stable map, JIT compilation can eliminate the static factory method call entirely, and the benchmark would be measuring the performance of no-op invocation.

I decided to remove this benchmark, as the fix is merely to revert the previous fix and not providing any performance improvement (to the original).

liach · 2024-12-21T00:56:46Z

The putIfAbsent remark from Roger Riggs applies to DateTimeTextProvider and DecimalStyle too. I think reusing existing result in these two places is beneficial, as the replaced computeIfAbsent returns the same object identity which may be helpful for quick equals comparisons.

bokken · 2024-12-26T16:39:58Z

src/java.base/share/classes/java/time/format/DateTimeTextProvider.java

+            store = createStore(field, locale);
+            CACHE.putIfAbsent(key, store);
+            store = CACHE.get(key);


should this be
store = CACHE.computeIfAbsent(key, e -> createStore(e.getKey(), e.getValue()));

That still allow the optimistic/concurrent get call to succeed most of the time (when already cached) but reduce the interactions with the map when a value is created/set/accessed the first time.

Alternatively, the result of putIfAbsent could be checked/used to avoid the second call to get.

For sure we should use result of putIfAbsent. Let's do this for all cases. See how it was implemented in my first commit - 73a2f6c

For sure we should use result of putIfAbsent

Drive-by comment...

From what i can infer, the performance regression being addressed here is caused in part by the fact that (for example) ConcurrentHashMap.computeIfAbsent() provides an atomicity guarantee, which is a stronger requirement than is necessary here, and therefore by splitting up that call up into two separate, non-atomic get() and put() calls we get (counter-intuitively) faster execution time, even though there are more lines of code. Note putIfAbsent() also guarantees atomicity, so the same problem of slowness caused by "unnecessary atomicity" might occur with it as well.

Indeed, just noticed that both computeIfAbsent and putIfAbsent may acquire the lock when the key is present, while get never acquires a lock for read-only access.

Maybe the implementation was written back when locking was less costly (with biased locking, etc.). Now we might have to reconsider locking until we know for sure a plain get fails.

This scenario is discussed in Effective Java by Joshua Block. His observation then (java 5/6 time frame?) was optimistically calling get first and only calling putIfAbsent or computeIfAbsent if the get returned null was 250% faster, and this is because calls to put/compute ifAbsent have contention. There have been changes made to those methods since then to try to avoid synchronization when the key is already present, but the observation seems to confirm that the optimistic get call first is still faster (though a much smaller difference).

My comment was not to revert back to the prior change of just calling computeIfAbsent, but rather just to change the (expected rare) case when the first get returns null to replace the putIfAbsent and second get call with a single computeIfAbsent (or utilize the result of putIfAbsent to avoid the second call to get).

Thanks for your observations. I think Archie's analysis sounds right, although have not confirmed. Will use the result from putIfAbsent() for all cases.

RogerRiggs

Looks good to me.

turbanoff · 2025-01-02T19:51:19Z

src/java.base/share/classes/java/time/ZoneOffset.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.


Shouldn't be 2025 too?

This PR was published last year and ZoneOffset has not changed since then. So I think 2024 is fine

naotoj · 2025-01-06T17:03:45Z

Thank you for the reviews!
/integrate

openjdk · 2025-01-06T17:04:09Z

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

12700cb: 8346264: "Total compile time" counter should include time spent in failing/bailout compiles
dd81f8d: 8344079: Minor fixes and cleanups to compiler lint-related code
ccf3d57: 8346985: Convert test/jdk/com/sun/jdi/ClassUnloadEventTest.java to Class-File API
594e519: 8346984: Remove ASM-based benchmarks from Class-File API benchmarks
c027f2e: 8346983: Remove ASM-based transforms from Class-File API tests
e0695e0: 8346981: Remove obsolete java.base exports of jdk.internal.objectweb.asm packages
dfaa891: 8346569: Shenandoah: Worker initializes ShenandoahThreadLocalData twice results in memory leak
f1d85ab: 8346773: Fix unmatched brackets in some misc files
9393897: 8346260: Test "javax/swing/JOptionPane/bug4174551.java" failed because the font size of message "Hi 24" is not set to 24 in Nimbus LookAndFeel
e98f412: 8346922: TestVectorReinterpret.java fails without the rvv extension on RISCV fastdebug VM
... and 6 more: https://git.openjdk.org/jdk/compare/d3abf01c3e8236d37ec369429e17f35afeb7ab88...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-06T17:04:15Z

@naotoj Pushed as commit 9a60f44.

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

initial commit

3df7e9f

openjdk bot added the rfr Pull request is ready for review label Dec 20, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Dec 20, 2024

RogerRiggs reviewed Dec 20, 2024

View reviewed changes

src/java.base/share/classes/java/time/ZoneOffset.java Outdated Show resolved Hide resolved

naotoj and others added 2 commits December 20, 2024 12:54

Update src/java.base/share/classes/java/time/ZoneOffset.java

1e09b8b

Co-authored-by: Roger Riggs <Roger.Riggs@Oracle.com>

Fixed compile error

8dca103

wenshao reviewed Dec 20, 2024

View reviewed changes

liach suggested changes Dec 21, 2024

View reviewed changes

bokken reviewed Dec 26, 2024

View reviewed changes

naotoj added 2 commits January 2, 2025 09:22

Merge branch 'master' into JDK-8345668-ofTotalSeconds-perf-regression

9553ebd

Addresses review comments

4dac7ba

RogerRiggs approved these changes Jan 2, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 2, 2025

turbanoff approved these changes Jan 2, 2025

View reviewed changes

turbanoff reviewed Jan 2, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 6, 2025

openjdk bot closed this Jan 6, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 6, 2025

ExE-Boss mentioned this pull request Jan 9, 2025

8346036: Unnecessary Hashtable usage in javax.swing.text.html.parser.Entity #21831

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8345668: ZoneOffset.ofTotalSeconds performance regression #22854

8345668: ZoneOffset.ofTotalSeconds performance regression #22854

naotoj commented Dec 20, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 20, 2024

openjdk bot commented Dec 20, 2024 •

edited

Loading

openjdk bot commented Dec 20, 2024

mlbridge bot commented Dec 20, 2024 •

edited

Loading

wenshao Dec 20, 2024

wenshao Dec 20, 2024 •

edited

Loading

naotoj Dec 20, 2024 •

edited

Loading

wenshao Dec 20, 2024

liach Dec 21, 2024 •

edited

Loading

liach Dec 21, 2024

liach Dec 21, 2024

naotoj Jan 2, 2025

liach commented Dec 21, 2024

bokken Dec 26, 2024

turbanoff Dec 26, 2024

archiecobbs Dec 27, 2024

liach Dec 27, 2024

bokken Dec 30, 2024

naotoj Jan 2, 2025

RogerRiggs left a comment

turbanoff Jan 2, 2025

naotoj Jan 2, 2025

naotoj commented Jan 6, 2025

openjdk bot commented Jan 6, 2025

openjdk bot commented Jan 6, 2025

8345668: ZoneOffset.ofTotalSeconds performance regression #22854

8345668: ZoneOffset.ofTotalSeconds performance regression #22854

Conversation

naotoj commented Dec 20, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 20, 2024

openjdk bot commented Dec 20, 2024 • edited Loading

openjdk bot commented Dec 20, 2024

mlbridge bot commented Dec 20, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

wenshao Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

naotoj Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liach commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naotoj commented Jan 6, 2025

openjdk bot commented Jan 6, 2025

openjdk bot commented Jan 6, 2025

naotoj commented Dec 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 20, 2024 •

edited

Loading

mlbridge bot commented Dec 20, 2024 •

edited

Loading

wenshao Dec 20, 2024 •

edited

Loading

naotoj Dec 20, 2024 •

edited

Loading

liach Dec 21, 2024 •

edited

Loading