Performance test and fix for logger part of the logging system #24949
Conversation
This file contains 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
- The test checks mostly the GlassFishLogger class and several message formatting strategies - The test doesn't check handlers, formatters. - The LogCollectorHandler has new constructor allowing to set the capacity and wait timeout. - The logrb method was not overridden (was added to JUL in Java 9); when overriden, caused significant slowdown because of the stacktrace analysis. - This commit does not pass the test. Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- The stacktrace analysis is now done just when - it is detected that some handler/formatter uses its results - it is explicitly enabled Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
dmatej
changed the title
avpinchuk
approved these changes
May 10, 2024
pzygielo
reviewed
May 10, 2024
| * If this key is set to true, GJULE will detect the caller class and method from stacktrace, | ||
| * which is quite expensive operation affecting logging throughput. | ||
| * <p> | ||
| * If it is set to null, GJULE will not perform such detection. |
There was a problem hiding this comment.
I don't understand that. It says: if set to null but the code seems to check for false.
I'm not very familiar with properties handling here, but set to null is different case than not set, isn't it?
Just would like to know what are my options here 😁 and where the false from
if (Boolean.FALSE.toString().equalsIgnoreCase(value)) {
return false;
}fits in.
There was a problem hiding this comment.
You are right, I will take a sleep and I will read and fix it after myself :-)
Thanks for noticing that!
And I should add that to the documentation too.
|
Ok, yet once more to the Draft - missing documentation, and OneLineFormatter has printSource as true by default. |
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
pzygielo
approved these changes
May 12, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
Years ago I made some comparisons for JUL logging, but I never pushed those tests.
Yesterday I had some doubts about the best way to format log messages, so I created a test. Meanwhile I noticed that one method in logger was not overridden and when I overrode it, it started failing the test. Long story short, GlassFish usually uses multiple handlers in parallel, so GJULE pushes common parts to caller threads to avoid duplicating and even blocking on handlers and formatters. But that means that the early resolution should not do work which is not required later - especially it should not analyze stacktraces when the detected class and method is not used in any output.
Oh, the original idea - yeah, MessageFormat is really slow compared to "+", StringBuilder, even behind suppliers. String.format is quiet slow too.