Issue #4572 - Implementing old logging TAG_PAD as Message Alignment.#4997
Merged
joakime merged 5 commits intojetty-10.0.xfrom Aug 20, 2020
Merged
Issue #4572 - Implementing old logging TAG_PAD as Message Alignment.#4997joakime merged 5 commits intojetty-10.0.xfrom
joakime merged 5 commits intojetty-10.0.xfrom
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Closed
Closed
Contributor
|
Ooops wrong one |
gregw
reviewed
Jun 25, 2020
jetty-slf4j-impl/src/main/java/org/eclipse/jetty/logging/StdErrAppender.java
Show resolved
Hide resolved
gregw
approved these changes
Jun 25, 2020
Contributor
gregw
left a comment
There was a problem hiding this comment.
LGTM. But perhaps check if substring of a string previously created by repeat is faster than repeat each time. Probably down in the noise!
| builder.append(' '); | ||
| int padAmount = messageAlignColumn - builder.length(); | ||
| if (padAmount > 0) | ||
| builder.append(" ".repeat(padAmount)); |
Contributor
There was a problem hiding this comment.
I'm just a little cautious that repeat might be less efficient than taking a substring of a longer string.
Contributor
Author
There was a problem hiding this comment.
Fair enough, i'll see what the numbers show.
I did look at the implementation of both in java.lang.String.
- String.substring() results in a new String object, based on "this.value" byte[] array, with an array copy to a new array for the new String object.
- String.repeat() results in a new String object, based on "this.value" byte[] array, with an array copy to a new array for the new String object.
Contributor
Author
There was a problem hiding this comment.
@gregw here's the results of the IndentBenchmark (jmh) test.
Benchmark Mode Cnt Score Error Units
IndentBenchmark.testStringRepeatLarge thrpt 7 186589212.883 ± 14318698.518 ops/s
IndentBenchmark.testStringRepeatLarge:·gc.alloc.rate thrpt 7 7822.988 ± 601.504 MB/sec
IndentBenchmark.testStringRepeatLarge:·gc.alloc.rate.norm thrpt 7 88.000 ± 0.001 B/op
IndentBenchmark.testStringRepeatLarge:·gc.churn.G1_Eden_Space thrpt 7 7854.853 ± 3677.899 MB/sec
IndentBenchmark.testStringRepeatLarge:·gc.churn.G1_Eden_Space.norm thrpt 7 88.310 ± 40.367 B/op
IndentBenchmark.testStringRepeatLarge:·gc.count thrpt 7 18.000 counts
IndentBenchmark.testStringRepeatLarge:·gc.time thrpt 7 64.000 ms
IndentBenchmark.testStringRepeatSmall thrpt 7 343002605.870 ± 25715711.800 ops/s
IndentBenchmark.testStringRepeatSmall:·gc.alloc.rate thrpt 7 9150.062 ± 688.673 MB/sec
IndentBenchmark.testStringRepeatSmall:·gc.alloc.rate.norm thrpt 7 56.000 ± 0.001 B/op
IndentBenchmark.testStringRepeatSmall:·gc.churn.G1_Eden_Space thrpt 7 9078.653 ± 4249.875 MB/sec
IndentBenchmark.testStringRepeatSmall:·gc.churn.G1_Eden_Space.norm thrpt 7 55.546 ± 25.642 B/op
IndentBenchmark.testStringRepeatSmall:·gc.count thrpt 7 18.000 counts
IndentBenchmark.testStringRepeatSmall:·gc.time thrpt 7 73.000 ms
IndentBenchmark.testStringSubStringLarge thrpt 7 179315393.232 ± 109455137.398 ops/s
IndentBenchmark.testStringSubStringLarge:·gc.alloc.rate thrpt 7 7518.461 ± 4593.109 MB/sec
IndentBenchmark.testStringSubStringLarge:·gc.alloc.rate.norm thrpt 7 88.000 ± 0.001 B/op
IndentBenchmark.testStringSubStringLarge:·gc.churn.G1_Eden_Space thrpt 7 7535.104 ± 6436.079 MB/sec
IndentBenchmark.testStringSubStringLarge:·gc.churn.G1_Eden_Space.norm thrpt 7 87.144 ± 46.695 B/op
IndentBenchmark.testStringSubStringLarge:·gc.count thrpt 7 18.000 counts
IndentBenchmark.testStringSubStringLarge:·gc.time thrpt 7 51.000 ms
IndentBenchmark.testStringSubStringSmall thrpt 7 303492551.318 ± 41265934.036 ops/s
IndentBenchmark.testStringSubStringSmall:·gc.alloc.rate thrpt 7 8096.412 ± 1102.379 MB/sec
IndentBenchmark.testStringSubStringSmall:·gc.alloc.rate.norm thrpt 7 56.000 ± 0.001 B/op
IndentBenchmark.testStringSubStringSmall:·gc.churn.G1_Eden_Space thrpt 7 8215.101 ± 3847.187 MB/sec
IndentBenchmark.testStringSubStringSmall:·gc.churn.G1_Eden_Space.norm thrpt 7 56.618 ± 22.595 B/op
IndentBenchmark.testStringSubStringSmall:·gc.count thrpt 7 18.000 counts
IndentBenchmark.testStringSubStringSmall:·gc.time thrpt 7 43.000 ms
Contributor
Author
There was a problem hiding this comment.
When I look at the results, I see that String.repeat() is slightly faster (in terms of ops/s. 4% faster in large cases, 12% faster in the small cases).
And the GC is about the same.
But with a higher error-bars on the substring version.
Contributor
Author
There was a problem hiding this comment.
@gregw bump. could use a review of the benchmark results here.
Contributor
There was a problem hiding this comment.
repeat LGTM. I think there may be other places we can do the same.... but another PR.
…4572-message-indent
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
…4572-message-indent
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Alternate approach from PR #4994
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com