logging-log4j2
logging-log4j2 copied to clipboard
[draft/proof of concept] JTL memory optimizations
09e1bae: Avoids allocating new string objects when parameters are formatted, preferring to use the existing StringBuilder instance.
38b9650: Remove JsonWriter.formattableBuffer in favor of using the existing JsonWriter.stringBuilder field.
from the commit message:
JsonWriter avoids an additional StringBuilder for formatting in
favor of escaping in places.
It's worth some benchmarks to compare and contrast. In the ideal
case, strings don't contain characters that must be escaped, and no
memory copying is necessary. When escaping does occur, contents are
localized to an array and cache hits are likely. We may not need
to reach into main memory as frequently, unless we've escaped enough
characters to fill a cache line.
This commit is a proof of concept, it's not ideal that we're combining
two separate json string escaping implementations in JsonWriter:
`StringBuilders.escapeJson` and `JsonWriter.quoteString`
First and foremost, thanks so much @carterkozak for reviewing the changes and thinking along for improvements. I have really appreciated them.
ParameterizedMessage.deepToString() changes seem legitimate to me. That said, I would like to share a few opinions of mine triggered by this change set:
- Even though I was the one who started using
ParameterizedMessage.deepToString(), thinking about it again, I am not happy with the end result. This breaks the self-containment contract of the class. I would/might rather copy your (or all?) optimizations fromParameterizedMessage.deepToString()to somewhere inJsonWriter#writeString(). - Indeed it smells awkward to have JSON quoting routines in multiple locations within the code base. I had shared my concerns about it in a mailing list thread. But in a nutshell, why do we even have JSON quoting routines anywhere in the code base except
JsonWriter?JsonLayoutalready uses Jackson, hence it is covered. But for the rest... I think we should just keep them for backward compatibility and remove them inmaster.
Even though I was the one who started using ParameterizedMessage.deepToString(), thinking about it again, I am not happy with the end result. This breaks the self-containment contract of the class. I would/might rather copy your (or all?) optimizations from ParameterizedMessage.deepToString() to somewhere in JsonWriter#writeString().
It's worth considering whether or not we're solving the same problem as ParameterizedMessage -- I don't think there's any reason to produce divergent implementations if the goal is to write the string value of an object to a StringBuilder. If the implementations need to be subtly different, than I agree it's safer to define them separately :-)
Indeed it smells awkward to have JSON quoting routines in multiple locations within the code base. I had shared my concerns about it in a mailing list thread. But in a nutshell, why do we even have JSON quoting routines anywhere in the code base except JsonWriter? JsonLayout already uses Jackson, hence it is covered. But for the rest... I think we should just keep them for backward compatibility and remove them in master.
Yep, this is a draft PR and I don't think we would want to combine the two implementations, however I do think it's worth considering using a similar algorithm for more efficient encoding in JTL. The idea is instead of using a second StringBuilder for intermediate output, we can write directly onto the final stringbuilder, then efficiently escape data. The escapeJson function counts characters which need to be escaped, and scans backward from the escaped length moving characters into place in O(n) time without additional memory requirements.