-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8366224: Introduce DecimalDigits.appendPair for efficient two-digit formatting and refactor DateTimeHelper #26911
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
base: master
Are you sure you want to change the base?
Conversation
…\n\nThis change adds a new internal API to efficiently append two-digit integers\n(00-99) to a StringBuilder. It includes:\n- AbstractStringBuilder.appendPair(int i): The core implementation.\n- JavaLangAccess.appendPair(StringBuilder, int): For internal access.\n- System.JavaLangAccessImpl.appendPair(StringBuilder, int): Bridge to AbstractStringBuilder.\n- DecimalDigits.appendPair(StringBuilder, int): Public static utility method.\n\nImproved Javadoc comments for clarity and consistency across all new methods. Co-authored-by: Qwen-Coder <[email protected]>
… formatting\n\nThis change updates DateTimeHelper.formatTo methods for LocalDate and LocalTime\nto use the new DecimalDigits.appendPair method for formatting month, day, hour,\nminute, and second components. This improves code clarity and leverages the\nnewly introduced efficient two-digit integer appending functionality. Co-authored-by: Qwen-Coder <[email protected]>
👋 Welcome back swen! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Co-authored-by: Qwen-Coder <[email protected]> Refactored the year formatting logic to use subtraction instead of modulo for calculating the lower two digits, which can be slightly more efficient. Added a comment to clarify the safety of the input range for DecimalDigits.appendPair.
I've run performance tests comparing the baseline (7b9969d) and this PR's branch (appendPair_202508) using the Performance Comparison Results
The most significant improvements are seen in Tests were run on macOS ARM64 The performance gains are consistent with the optimization goals of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here looks reasonable to me, and the performance gain is nice.
Webrevs
|
* The integer {@code v} is formatted as two decimal digits. | ||
* If the value is between 0 and 9, it is formatted with a leading zero | ||
* (e.g., 5 becomes "05"). If the value is outside the range 0-99, | ||
* the behavior is unspecified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of interest, why leave the behaviour as unspecified instead of validating and throwing an exception?
Nice performance gains, but still this is quite a complex solution with a narrow use case (optimizes toString() of some java.time classes). Solution sketch: Introduce a new Latin1Builder
|
Generally, even with the performance benefits, this code is too narrow a use case to justify the hacking into AbstractStringBuilder and adding another package busting JavaLangAccess entry point. |
This change modifies the toString() methods in MonthDay, YearMonth, ZoneOffset, and ChronoLocalDateImpl to use DecimalDigits.appendPair for formatting two-digit numbers. This provides a more efficient and consistent way to format these values. Also added a comment in ChronoLocalDateImpl.toString() to explain why get() is used instead of getLong() for performance reasons, as the values are guaranteed to be within the int range for all chronologies. Co-authored-by: Qwen-Coder <[email protected]>
Thank you for your feedback, @RogerRiggs. I introduced this PR because I plan to optimize the performance of In future optimizations, I need to use In the current PR, using
month = date.getMonthValue(),
day = date.getDayOfMonth();
buf.append(month < 10 ? "-0" : "-").append(month)
.append(day < 10 ? "-0" : "-").append(day);
buf.append('-');
DecimalDigits.appendPair(buf, date.getMonthValue());
buf.append('-');
DecimalDigits.appendPair(buf, date.getDayOfMonth()); The improved code is more readable than the original. Placing
|
This PR introduces a new efficient API for appending two-digit integers to StringBuilders and refactors DateTimeHelper to leverage this new functionality.
Changes include:
New
appendPair
method for efficient two-digit integer formatting (00-99):AbstractStringBuilder.appendPair(int i)
with core implementationJavaLangAccess.appendPair(StringBuilder, int)
for internal accessSystem.JavaLangAccessImpl.appendPair(StringBuilder, int)
bridgeDecimalDigits.appendPair(StringBuilder, int)
public static utility methodRefactored
DateTimeHelper
to use the newDecimalDigits.appendPair
:DateTimeHelper.formatTo
methods forLocalDate
andLocalTime
These changes improve code clarity and performance when formatting two-digit numbers, particularly in date/time formatting scenarios.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26911/head:pull/26911
$ git checkout pull/26911
Update a local copy of the PR:
$ git checkout pull/26911
$ git pull https://git.openjdk.org/jdk.git pull/26911/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26911
View PR using the GUI difftool:
$ git pr show -t 26911
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26911.diff
Using Webrev
Link to Webrev Comment