Improve performance of DefaultHttpLogFormatter #524
Conversation
skjolber
requested review from
AlexanderYastrebov,
lukasniemeier-zalando and
whiskeysierra
as code owners
skjolber
force-pushed
the
jmhHttpLogFormatter
branch
2 times, most recently
from
dc1d5ed to
af05b0d
Compare
…book into jmhHttpLogFormatter
skjolber
force-pushed
the
jmhHttpLogFormatter
branch
from
af05b0d to
9bf4d2a
Compare
The README states that
I'm not saying that we shouldn't take low-hanging fruits and make it faster, but if it requires semi-significant changes to our API (seems to me that it does) then we should re-consider. |
skjolber
force-pushed
the
jmhHttpLogFormatter
branch
from
a1fef28 to
7f0deb4
Compare
skjolber
force-pushed
the
jmhHttpLogFormatter
branch
from
7f0deb4 to
2e64091
Compare
|
Visibility changes to API should be reverted now, must had leaked from previously adapting JsonHttpLogFormatter in the same branch. 👀 My initial description is incorrect, DefaultHttpLogFormatter does not currently use StructuredHttpLogFormatter, so this PR is essentially moving to directly writing with a StringBuilder instead of collecting a list of lines and appending them at the end. I'll admit the performance of the HttpLogFormatter is not that important, but at least it is quite a lot faster now (like 10x). We're actually running both the HttpLogFormatter and the JsonHttpLogFormatter for build / local development, one is printed to console and the other is used for log statement unit testing. |
|
BTW: Has Ansi coding been discussed? |
I actually don't mind having this change. The code is still readable overall. 10x is quite appealing, tbh.
What do you mean? |
| } | ||
|
|
||
| private static boolean isNotStandardPort(final String scheme, final int port) { | ||
| return "http".equals(scheme) && port != 80 || | ||
| "https".equals(scheme) && port != 443; | ||
| return ("http".equals(scheme) && port != 80) || |
There was a problem hiding this comment.
Those are not really needed, are they?
There was a problem hiding this comment.
Strictly, no, it was intended as an improvement in readability, forgot to comment.
|
Ansi coding as in colors for improved readablity for local development and debugging. I guess it could be a future improvement (i.e. enabled by a config parameter). |
Let's move this to a separate issue. |
|
👍 |
Improve performance of DefaultHttpLogFormatter.
Description
Construct log statement in a single 'streaming' fasion.
Motivation and Context
The previous implementation used a two-step approach via StructuredHttpLogFormatters format + prepare. Switching to a single step approach improves performance considerably, run HttpLogFormatterBenchmark for comparisons.
Types of changes
Checklist: