Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[resubmit] BigInteger parsing optimization for large decimal string #55121
[resubmit] BigInteger parsing optimization for large decimal string #55121
Changes from all commits
e93dde5
2fdbe45
7d89c46
417a8a3
eb7be98
8f79da7
9a432db
fce80e8
f6eadca
c3903db
a9942c5
460664f
607f51c
7f9fcd8
470d761
b1c717b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Given how big each path is, I would if it would be better to break them into 2 helper methods. Basically leaving:
-- The method is getting pretty big, which means the JIT might give up on optimizing it otherwise (haven't confirmed if it actually does).
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.
Sorry for late replying. I think this is worth doing in terms of improving readability. I will implement it as soon as possible.
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 have implemented and benchmarked. As a result, there is no significant difference in speed and memory. However, readability has definitely been improved.
Benchmark result
Job-VEBSQX
: before split to methods (a9942c5)Job-HLAVXS
: after split to methods (460664f)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.
If this loop is executed
ceil(log_2(bufferSize))
, why do you not use a for loop? I think these are better optimized by the JIT.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.
Please benchmark before/after to make sure that this is really the case.