On Fri, 25 Apr 2025 01:36:50 GMT, Shaojin Wen <s...@openjdk.org> wrote:
> In the Throwable::printStackTrace method, StringBuilder is created multiple > times to build String. By sharing StringBuilder to build String, object > allocation and copying are reduced. > > In the scenario without suppressed and ourCause, unused IdentityHashMap is > not created. > > Through these optimizations, the performance of `new > Exception().printStackTrace()` can be improved by about 10%. What about this version, we can reuse the StringBuilder for the enclosed because we are behind the lock. Also avoids changing caption and prefix. Edit: removed patch to avoid mail spam, look at edit history or the gist linked below I have thought about the prefix and caption again - the prefix is just an indent. If we reuse the buffer for line, we just reset position to the end of indent, and you don't need to worry about concating caption and prefix (actually indent) any more. https://gist.github.com/0974e48fb01c8c2ce290ab3ced36dcc2 src/java.base/share/classes/java/lang/StackTraceElement.java line 367: > 365: } > 366: > 367: int estimatedLength() { Don't think you need to split this into a new method; you can just keep the old variable as the calculation is too long. src/java.base/share/classes/java/lang/StackTraceElement.java line 381: > 379: */ > 380: StringBuilder appendTo(StringBuilder sb) { > 381: int length = sb.length(); Can we rename this to `startingLength` for clarity? src/java.base/share/classes/java/lang/Throwable.java line 696: > 694: printStackTrace0(s, trace); > 695: > 696: java.lang.Throwable[] suppressed = getSuppressed(); Suggestion: Throwable[] suppressed = getSuppressed(); src/java.base/share/classes/java/lang/Throwable.java line 705: > 703: java.lang.Throwable[] suppressed = getSuppressed(); > 704: if (suppressed.length > 0) { > 705: dejaVu = dejaVu(); Instead of creating dejaVu() here and there, I think you can do after printStackTrace0: printStackTrace0(s, trace); var suppressed = getSuppressed(); var ourCause = getCause(); if (suppressed.length > 0 || cause != null) { // Guard against malicious overrides of Throwable.equals by // using a Set with identity equality semantics. Set<Throwable> dejaVu = Collections.newSetFromMap(new IdentityHashMap<>()); dejaVu.add(this); // more code here } src/java.base/share/classes/java/lang/Throwable.java line 749: > 747: // Print suppressed exceptions, if any > 748: for (Throwable se : getSuppressed()) > 749: se.printEnclosedStackTrace(s, trace, SUPPRESSED_CAPTION, Shouldn't SUPPRESSED_CAPTION be added a prefix? I think we should revert the `prefixCaption` argument change; you should add a line `String prefixCaption = prefix.concat(caption);` instead. src/java.base/share/classes/java/lang/Throwable.java line 785: > 783: StackTraceElement[] trace, > 784: int traceEnd, > 785: int framesInCommon) { You just need to pass `framesInCommon`, can calculate `int limit = trace.length - framesInCommon;` and then for the loop below, `for (int i = 0; i < limit; i++)` instead of using `traceEnd` src/java.base/share/classes/java/lang/Throwable.java line 804: > 802: > 803: var sb = new StringBuilder(128); > 804: if (prefix != null) { I think you should can this whole thing to be cleaner: var sb = new StringBuilder(128); if (prefix != null) sb.append(prefix); sb.append(at); int prefixAndAtLength = sb.length(); for (int i = 0; i <= traceEnd; i++) { sb.setLength(prefixAndAtLength); printer.println(trace[i].appendTo(sb)); } if (framesInCommon != 0) { sb.setLength(prefixAndAtLength - at.length()) .append("\t... ").append(framesInCommon).append(" more"); printer.println(sb); } ------------- PR Comment: https://git.openjdk.org/jdk/pull/24864#issuecomment-2829331303 PR Comment: https://git.openjdk.org/jdk/pull/24864#issuecomment-2829349920 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059504832 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059499587 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059499808 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059444188 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059502886 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059503902 PR Review Comment: https://git.openjdk.org/jdk/pull/24864#discussion_r2059447895
