-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
8291641: Optimize StackTraceElement.toString() #9665
Conversation
👋 Welcome back schlosna! A progress list of the required criteria for merging this PR into |
I think there's another angle to consider here: It would be helpful for my uses if the new internal attributes were exposed in some way from StackTraceElement, however I understand why that isn't desirable from an API design standpoint. |
If the objective of this patch is to optimise And if you want to propose exposing Thanks. |
0c27cb3
to
de0a337
Compare
de0a337
to
aca4428
Compare
Thanks for the reviews and comments! To @merykitty 's point, I am now focusing this PR on optimizing I tested out a couple alternative implementations, and have currently landed on precomputing the StringBuilder capacity as seen in current state of this PR (commit aca4428 ). This shaves a bit more off the
Alternately, I computed the length and collected into a
I did not write up an implementation to buffer & encode to a |
@schlosna I've filed https://bugs.openjdk.org/browse/JDK-8291641 for this. Please use id |
Webrevs
|
Since constructing a |
@schlosna Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information. |
Good call @merykitty , I updated with my take on simplifying the length estimate, let me know if that reads better. JMH as of d3774a6 for comparison to previous, so still a ~50% improvement over existing state on my x64 test environment (
|
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.
Otherwise looks good to me, thanks
+ declaringClass.length() + 1 | ||
+ methodName.length() + 1 | ||
+ Math.max("Unknown Source".length(), Objects.requireNonNullElse(fileName, "").length()) + 1 | ||
+ Integer.stringSize(lineNumber) + 1; |
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.
Integer.stringSize(lineNumber)
can be further simplify to 11
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.
inlined as 12 (combined with 1
for trailing )
)
Are there existing tests that validate the formatted results? |
Thanks @RogerRiggs , I added the bug ID to the existing StackTraceElement SerialTest which validates the structure of jdk/test/jdk/java/lang/StackTraceElement/SerialTest.java Lines 26 to 27 in c93ac5a
|
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.
LGTM
@schlosna 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:
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 35 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
/integrate |
@@ -355,27 +354,46 @@ public boolean isNativeMethod() { | |||
* @revised 9 | |||
* @see Throwable#printStackTrace() | |||
*/ | |||
@Override |
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 override makes no difference.
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.
ack. I can remove if necessary, but stylistically prefer to mark overridden methods as such (and automatically enforce via static analysis )
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.
Looks good to me. Minor suggestions.
+ Objects.requireNonNullElse(moduleVersion, "").length() + 1 | ||
+ declaringClass.length() + 1 | ||
+ methodName.length() + 1 | ||
+ Math.max("Unknown Source".length(), Objects.requireNonNullElse(fileName, "").length()) + 1 |
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 suggest to define a static variable = "Unknown Source" be used here and line 387. Same for "Native Method" which can also be accounted for.
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.
pulled out to private static final fields
if (!dropClassLoaderName() && classLoaderName != null && | ||
!classLoaderName.isEmpty()) { | ||
s += classLoaderName + "/"; | ||
int estimatedLength = Objects.requireNonNullElse(classLoaderName, "").length() + 1 |
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.
Nit: suggest to refactor this and define a private static length(String s)
method.
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.
updated
/integrate |
/sponsor |
Going to push as commit 9825c33.
Your commit was automatically rebased without conflicts. |
I would like to contribute an optimized version of
StackTraceElement#toString()
that uses a single StringBuilder throughout creation to avoid intermediateString
allocations.StackTraceElement#toString()
is used in a number of JDK code paths includingThrowable#printStackTrace()
, as well as many JDK consumers may transformStackTraceElement
toString()
in logging frameworks capturing throwables and exceptions, and diagnostics performing dumps.Given this usage and some observed JFR profiles from production services, I'd like to reduce the intermediate allocations to reduce CPU pressure in these circumstances. I have added a couple benchmarks for a sample
Throwable#printStackTrace()
converted to String viaStringWriter
and individualStackTraceElement
toString
. The former shows ~15% improvement, while the latter shows ~40% improvement.Before
After
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9665/head:pull/9665
$ git checkout pull/9665
Update a local copy of the PR:
$ git checkout pull/9665
$ git pull https://git.openjdk.org/jdk pull/9665/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9665
View PR using the GUI difftool:
$ git pr show -t 9665
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9665.diff