8337279: Share StringBuilder to format instant by wenshao · Pull Request #20353 · openjdk/jdk

wenshao · 2024-07-26T14:20:07Z

Create a tool class jdk.internal.util.DateTimeHelper, move the formatTo method of LocalDateTime/LocalDate/LocalTime to it, so that these methods can be used across packages within JDK, so that StringBuilder can be shared, avoiding multiple creation of StringBuilder and toString.
Refactor DateTimeFormatterBuilder::format to use the jdk.internal.util.DateTimeHelper::formatTo method.
Split the DateTimeFormatterBuilder::format method, separate the currentEra and beforeCurrentEra sub-methods, so that codeSize < 325 can be inlined. Most scenarios call currentEra, so performance is improved.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8337279: Share StringBuilder to format instant (Enhancement - P4)

Reviewers

Naoto Sato (@naotoj - Reviewer)
Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20353

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-26T14:20:56Z

👋 Welcome back wenshao! 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-07-26T14:22:37Z

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

8337279: Share StringBuilder to format instant

Reviewed-by: naoto, liach

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

22182f7: 8352112: [ubsan] hotspot/share/code/relocInfo.cpp:130:37: runtime error: applying non-zero offset 18446744073709551614 to null pointer
c2e14b1: 8343891: Test javax/swing/JTabbedPane/TestJTabbedPaneBackgroundColor.java failed
52c6ce6: 8352091: GenShen: assert(!(request.generation->is_old() && _heap->old_generation()->is_doing_mixed_evacuations())) failed: Old heuristic should not request cycles while it waits for mixed evacuation
b8f3856: 8352420: [ubsan] codeBuffer.cpp:984:27: runtime error: applying non-zero offset 18446744073709486080 to null pointer
0cb110e: 8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields
04eac0c: 8352159: RISC-V: add more zfa support
ac760dd: 8352423: RISC-V: simplify DivI/L ModI/L
d1cf232: 8352248: Check if CMoveX is supported
2b55979: 8352529: RISC-V: enable loopopts tests
8469458: 8352511: Show additional level of headings in table of contents
... and 603 more: https://git.openjdk.org/jdk/compare/f98d9a330128302207fb66dfa2555885ad93135f...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 · 2024-07-26T14:22:59Z

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

wenshao · 2024-07-26T14:25:23Z

By removing redundant code logic, the codeSize can be reduced from 444 to 240, and the performance is improved.

Below are the performance numbers running on MacBook M1 Pro

-# master 5ff7c57f9ff5428ef3d2aedd7e860bb1e8ff29ea
-Benchmark                       Mode  Cnt  Score   Error   Units
-ToStringBench.instantToString  thrpt   15  4.734 ? 0.547  ops/ms +24.10%

+# current baf0f9ca83c111401d6e034dfba0ee2c71280e62
+Benchmark                       Mode  Cnt  Score   Error   Units
+ToStringBench.instantToString  thrpt   15  5.875 ? 0.301  ops/ms

mlbridge · 2024-07-26T14:29:45Z

Webrevs

jodastephen · 2024-07-26T15:24:46Z

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

            }
            // add fraction
-            if ((fractionalDigits < 0 && inNano > 0) || fractionalDigits > 0) {
+            if (fractionalDigits > 0) {


This breaks the logic. fractionalDigits can be negative in the block below

If fractionalDigits < 0, printNano is implemented in LocalDateTime

fractionalDigits == -2 is used to output 0, 3, 6 or 9 fractional digits as needed. This can be handled by LocalDateTime.

fractionalDigits == -1 is used to output as many fractional digits as needed, from 0 to 9 digits. This is NOT handled by LocalDateTime.

Furthermore, if the -2 branch is handled by LocalDateTime then the logic below for -2 is redundant.

If the tests did not fail as a result of this change, then I imagine the tests need improving.

plokhotnyuk · 2024-07-26T15:53:11Z

@jodastephen @wenshao Could you please review these proposed changes for instantiation of Instant values with already normalized nanos?

RogerRiggs · 2024-07-26T18:03:18Z

Could you please review these proposed changes for instantiation of Instant values with already normalized nanos?

@plokhotnyuk Your contribution cannot be acted upon until the OCA is confirmed.
If you have not submitted an OCA, please do so. See https://oca.opensource.oracle.com/

plokhotnyuk · 2024-07-27T19:22:49Z

Could you please review these proposed changes for instantiation of Instant values with already normalized nanos?

@plokhotnyuk Your contribution cannot be acted upon until the OCA is confirmed. If you have not submitted an OCA, please do so. See https://oca.opensource.oracle.com/

@RogerRiggs Thanks for your support!

I've submitted a signed OCA more than year ago and nobody answered me yet.

Today's attempt shows the following notification:

jodastephen · 2024-07-28T13:34:30Z

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

            }
            // add fraction
-            if ((fractionalDigits < 0 && inNano > 0) || fractionalDigits > 0) {
+            if (fractionalDigits > 0) {


fractionalDigits == -2 is used to output 0, 3, 6 or 9 fractional digits as needed. This can be handled by LocalDateTime.

fractionalDigits == -1 is used to output as many fractional digits as needed, from 0 to 9 digits. This is NOT handled by LocalDateTime.

Furthermore, if the -2 branch is handled by LocalDateTime then the logic below for -2 is redundant.

If the tests did not fail as a result of this change, then I imagine the tests need improving.

jodastephen · 2024-07-28T13:36:48Z

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

-                    }
-                }
+            // use LocalDateTime.toString, If fractionalDigits < 0, printNano is implemented in LocalDateTime
+            LocalDateTime ldt = LocalDateTime.ofEpochSecond(inSecs, fractionalDigits >= 0 ? 0 : inNano, ZoneOffset.UTC);


I believe this whole approach is flawed. The comment above says:

// use INSTANT_SECONDS, thus this code is not bound by Instant.MAX

which indicates that the instant value may not fit in LocalDateTime.

You may be able to special case certain code paths to use LocalDateTime, but that makes things more complex, and not necessarily faster.

2. Split two methods to make codeSize less than 325

wenshao · 2024-07-28T17:06:47Z

@jodastephen thank you for your patience in answering my questions.

Now I've used a new way to implement it, splitting it into two sub-methods without changing the original logic, which also makes codeSize < 325, and the performance is also faster than before. Here are the performance numbers running on a MacBook M1 Pro:

- # master 5ff7c57f9ff5428ef3d2aedd7e860bb1e8ff29ea
-Benchmark                       Mode  Cnt  Score   Error   Units
-ToStringBench.instantToString  thrpt   15  4.558 ? 0.561  ops/ms

+ # current 757984258257fd82c389f3ee7bd9be927375e2e0
+Benchmark                       Mode  Cnt  Score   Error   Units
+ToStringBench.instantToString  thrpt   15  5.564 ? 0.465  ops/ms +22.07%

I also added relevant tests to prevent errors from being discovered in future related changes.

I hope to reuse LocalDateTime.toString for the most common paths, so that we can also use LocalDateTime.formatTo method, which can reduce object allocation.

I plan to continue to optimize the formatTo method of LocalTime and LocalDate, so that InstantPrinterParser.format can also benefit from it.

jodastephen

I'm still skeptical of some parts of this PR as it makes the code harder to folow. The new tests are a good addition and should be merged.

Have you tried the performance of simply breaking out the currentEra/beforeCurrentEra methods without making any other changes? Since the logic to produce the nano fraction is going to stay in this method, I don't really see the advantage in trying to get LocalDateTime to do the fraction some of the time.

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

…Builder.java Co-authored-by: Stephen Colebourne <scolebourne@joda.org>

jodastephen · 2024-08-11T10:13:35Z

A more productive direction might be to move LocalDate.formatTo and LocalTime.formatTo to a JDK internal class (eg. in jdk.internal.util?), and then edit LocalTime.formatTo to handle fractionalDigits directly as another method parameter. This would allow the formatters in DateTimeFormatterBuilder to directly use the formatTo methods without adding any new public APIs.

wenshao · 2024-08-11T11:13:34Z

I'm still skeptical of some parts of this PR as it makes the code harder to folow. The new tests are a good addition and should be merged.

Have you tried the performance of simply breaking out the currentEra/beforeCurrentEra methods without making any other changes? Since the logic to produce the nano fraction is going to stay in this method, I don't really see the advantage in trying to get LocalDateTime to do the fraction some of the time.

git remote add wenshao git@github.com:wenshao/jdk.git
git fetch wenshao

# baseline with test
git checkout 85ee5de07dd9f45a4e1659e17f9a52e69fa77df3
make test TEST="micro:java.time.ToStringBench.instantToString"

Benchmark                       Mode  Cnt  Score   Error   Units
ToStringBench.instantToString  thrpt   15  4.749 ? 0.624  ops/ms

# current version
git checkout 875666143e6d717634f2a3cc3c397b2ca8b63e63
make test TEST="micro:java.time.ToStringBench.instantToString"

Benchmark                       Mode  Cnt  Score   Error   Units
ToStringBench.instantToString  thrpt   15  5.713 ? 0.474  ops/ms +20.29%

# baseline with simply breaking out the currentEra/beforeCurrentEra methods without making any other changes
git fbf66307f738cd40e061c6d26fa05c062ccd048b
make test TEST="micro:java.time.ToStringBench.instantToString"

Benchmark                       Mode  Cnt  Score   Error   Units
ToStringBench.instantToString  thrpt   15  4.716 ? 0.569  ops/ms -0.69%

In the branch fbf66307f738cd40e061c6d26fa05c062ccd048b, the benchmark results are very unstable because the test data includes currentEra and beforeCurrentEra. The current version has the best performance and can handle currentEra and beforeCurrentEra

The result is somewhat unexpected. I thought the performance improvement was mainly due to codeSize < 325. The specific reason needs further analysis.

wenshao · 2024-08-11T11:40:36Z

A more productive direction might be to move LocalDate.formatTo and LocalTime.formatTo to a JDK internal class (eg. in jdk.internal.util?), and then edit LocalTime.formatTo to handle fractionalDigits directly as another method parameter. This would allow the formatters in DateTimeFormatterBuilder to directly use the formatTo methods without adding any new public APIs.

I prefer to provide JavaTimeAccess in SharedSecrets, which will require less changes. This depends on #20368

…_202407

jodastephen

The change looks OK from my perspective, however I would want someone with experience of SharedSecrets to comment

wenshao · 2025-01-30T14:02:35Z

/open

openjdk · 2025-01-30T14:03:11Z

@wenshao This pull request is now open

j3graham · 2025-01-30T14:22:04Z

src/java.base/share/classes/jdk/internal/util/DateTimeHelper.java

+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.internal.util;


Maybe jdk.internal.time would be appropriate.

Currently, since we only have the DateTimeHelper class, we can wait until there are two or more classes before creating the jdk.internal.time package.

This utility class can be final, and have a private default constructor so that no instance can be created.

RogerRiggs · 2025-01-30T14:57:24Z

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

 import static java.time.LocalTime.NANOS_PER_SECOND;
 import static java.time.LocalTime.SECONDS_PER_DAY;
 import static java.time.temporal.ChronoField.NANO_OF_SECOND;
+import static jdk.internal.util.DateTimeHelper.formatTo;


Do not import static across package boundaries. I discourage it even within the same package.
It makes the code hard to read and follow where the function is coming from.

wenshao · 2025-02-01T00:39:19Z

I don't think using SharedSecret just for performance improvement and code size reduction is the right way, as the class is the last resort as the header says:
 * <p><strong>
 * Usage of these APIs often means bad encapsulation designs,
 * increased complexity and lack of sustainability.
 * Use this only as a last resort!
 * </strong>

@naotoj
I have removed SharedSecrets, can you review it again?

naotoj

Looks good with some minor nits

naotoj · 2025-02-03T22:10:22Z

src/java.base/share/classes/jdk/internal/util/DateTimeHelper.java

+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.internal.util;


This utility class can be final, and have a private default constructor so that no instance can be created.

naotoj · 2025-02-03T22:11:17Z

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

+            bh.consume(instant.toString());
+        }
+    }
 }


Please add a new line at the end

wenshao · 2025-02-03T22:30:06Z

Thank you @naotoj, these have been fixed, please help review again

naotoj

Looks good. I confirmed the changes passed tier1-3 tests.

AlanBateman · 2025-02-05T12:00:31Z

/reviewers 2 reviewer

openjdk · 2025-02-05T12:00:53Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

bridgekeeper · 2025-03-05T17:57:41Z

@wenshao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

wenshao · 2025-03-09T15:40:34Z

Keep it alive.

liach

Refactor looks harmless. Optimization is splitting methods to help profiling and avoid one string copy in ZonedDateTime.

wenshao · 2025-03-22T01:34:15Z

/integrate

openjdk · 2025-03-22T01:35:12Z

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

22182f7: 8352112: [ubsan] hotspot/share/code/relocInfo.cpp:130:37: runtime error: applying non-zero offset 18446744073709551614 to null pointer
c2e14b1: 8343891: Test javax/swing/JTabbedPane/TestJTabbedPaneBackgroundColor.java failed
52c6ce6: 8352091: GenShen: assert(!(request.generation->is_old() && _heap->old_generation()->is_doing_mixed_evacuations())) failed: Old heuristic should not request cycles while it waits for mixed evacuation
b8f3856: 8352420: [ubsan] codeBuffer.cpp:984:27: runtime error: applying non-zero offset 18446744073709486080 to null pointer
0cb110e: 8350892: [JVMCI] Align ResolvedJavaType.getInstanceFields with Class.getDeclaredFields
04eac0c: 8352159: RISC-V: add more zfa support
ac760dd: 8352423: RISC-V: simplify DivI/L ModI/L
d1cf232: 8352248: Check if CMoveX is supported
2b55979: 8352529: RISC-V: enable loopopts tests
8469458: 8352511: Show additional level of headings in table of contents
... and 603 more: https://git.openjdk.org/jdk/compare/f98d9a330128302207fb66dfa2555885ad93135f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-22T01:35:19Z

@wenshao Pushed as commit 7442039.

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

optimize format instant

baf0f9c

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Jul 26, 2024

wenshao changed the title ~~Optimize format instant~~ 8337279: Optimize format instant Jul 26, 2024

openjdk bot added the rfr Pull request is ready for review label Jul 26, 2024

jodastephen reviewed Jul 26, 2024

View reviewed changes

add comment

64403d5

jodastephen reviewed Jul 28, 2024

View reviewed changes

1. fix handle fraction == -1

7579842

2. Split two methods to make codeSize less than 325

wenshao requested a review from jodastephen August 8, 2024 05:09

jodastephen reviewed Aug 11, 2024

View reviewed changes

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

wenshao and others added 2 commits August 11, 2024 18:09

Update src/java.base/share/classes/java/time/format/DateTimeFormatter…

a5abb54

…Builder.java Co-authored-by: Stephen Colebourne <scolebourne@joda.org>

copyright

8756661

wenshao added 4 commits August 11, 2024 21:21

breaking out the printNano methods

ccbd6b5

Merge remote-tracking branch 'upstream/master' into optim_instant_fmt…

958da50

…_202407

add JavaTimeAccess SharedSecrets

a77b4a3

Speed up Instant.toString using JavaTimeAccess

df2a13d

jodastephen reviewed Aug 31, 2024

View reviewed changes

openjdk bot reopened this Jan 30, 2025

wenshao added 2 commits January 30, 2025 22:10

copyright

ad48758

remove unused import

c3b9dd5

j3graham reviewed Jan 30, 2025

View reviewed changes

RogerRiggs reviewed Jan 30, 2025

View reviewed changes

do not import static, from @RogerRiggs

5924935

wenshao requested a review from RogerRiggs February 1, 2025 00:40

naotoj reviewed Feb 3, 2025

View reviewed changes

from @natoj

5ca4d6e

naotoj approved these changes Feb 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 4, 2025

openjdk bot removed the ready Pull request is ready to be integrated label Feb 5, 2025

wenshao changed the title ~~8337279: Optimize format instant~~ 8337279: Share StringBuilder to format instant Mar 21, 2025

liach approved these changes Mar 22, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 22, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 22, 2025

openjdk bot closed this Mar 22, 2025

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

Conversation

wenshao commented Jul 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 26, 2024

Uh oh!

openjdk bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 26, 2024

Uh oh!

wenshao commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plokhotnyuk commented Jul 26, 2024

Uh oh!

RogerRiggs commented Jul 26, 2024

Uh oh!

plokhotnyuk commented Jul 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jul 28, 2024

Uh oh!

jodastephen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jodastephen commented Aug 11, 2024

Uh oh!

wenshao commented Aug 11, 2024

Uh oh!

wenshao commented Aug 11, 2024

Uh oh!

jodastephen left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jan 30, 2025

Uh oh!

openjdk bot commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenshao commented Feb 1, 2025

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenshao commented Feb 3, 2025

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Feb 5, 2025

wenshao commented Jul 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 26, 2024 •

edited

Loading

wenshao commented Jul 26, 2024 •

edited

Loading

mlbridge bot commented Jul 26, 2024 •

edited

Loading