-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8315585: Optimization for decimal to string #23310
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
|
👋 Welcome back swen! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
| * @param buf target buffer, UTF16-coded. | ||
| * @return index of the most significant digit or minus sign, if present | ||
| */ | ||
| public static int getChars(long i, int index, char[] buf) { |
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.
Before dropping this method, there is another candidate to use it here :
| private void developLongDigits( int decExponent, long lvalue, int insignificantDigits ){ |
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.
I took a look at the FloatingDecimal related code. The relevant code in FloatingDecimal can be refactored and simplified so that the removed code does not need to be used.
I submitted a PR to simplify FloatingDecimal #23311
|
A great cleanup that consolidates scale2, unscaledAbsString, and unscaledString. |
| } else if (insertionPoint > 0) { /* Point goes inside intVal */ | ||
| buf = new StringBuilder(intString); | ||
| buf.insert(insertionPoint, '.'); | ||
| buf = new StringBuilder(); |
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.
Could calculate the precise size for the StringBuilder
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 performance will degrade if you precompute the length of the StringBuilder.
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.
That’s interesting - should the precomputed lengths be removed in the other ones as well?
| buf.insert(0, '-'); | ||
| buf.append('-'); | ||
| buf.append(intString) | ||
| .insert(insertionPoint + (signum < 0 ? 1 : 0), '.'); |
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.
Instead of the insert, could do an append of the prefix, then dot, then the suffix.
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.
buf.append(intString, 0, insertionPoint)
.append('.')
.append(intString, insertionPoint, intString.length());This is another way to write it, but the performance will be reduced.
| } | ||
|
|
||
| private String layoutCharsE(boolean sci, String coeff, int coeffLen, long adjusted) { | ||
| StringBuilder buf = new StringBuilder(32); |
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 comment above about constructing a buffer probably belongs here. Also could calculate a better approximation of the size - possibly coeffLen+14 if that comment is accurate.
This reverts commit ddc2a86.
|
@wenshao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
# Conflicts: # src/java.base/share/classes/java/math/BigDecimal.java # src/java.base/share/classes/jdk/internal/util/DecimalDigits.java # test/micro/org/openjdk/bench/java/math/BigDecimals.java
|
/open |
|
@wenshao This pull request is now open |
|
@wenshao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
|
/touch |
|
@wenshao The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
@wenshao this pull request can not be integrated into git checkout dec_to_str_202501
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
# Conflicts: # src/java.base/share/classes/java/math/BigDecimal.java
…Latin1Bytes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@wenshao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
|
/touch |
|
@wenshao The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
@wenshao This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a |
|
/touch |
|
@wenshao The pull request is being re-evaluated and the inactivity timeout has been reset. |
| buf.append(coeff.charAt(0)); // first character | ||
| if (coeffLen > 1) { // more to come | ||
| buf.append('.') | ||
| .append(coeff, 1, coeffLen); |
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.
Is this right that the old end is coeffLen - 1 but the new end is coeffLen? I don't see why there should be no more -1.
Continue to complete PR #16006 and PR #21593 to improve BigDecimal::toString and BigDecimal::toPlainString performance and reduce duplicate code
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23310/head:pull/23310$ git checkout pull/23310Update a local copy of the PR:
$ git checkout pull/23310$ git pull https://git.openjdk.org/jdk.git pull/23310/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23310View PR using the GUI difftool:
$ git pr show -t 23310Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23310.diff
Using Webrev
Link to Webrev Comment