You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/11/29 16:18:02 UTC
[GitHub] [logging-log4j2] carterkozak opened a new pull request #444: [draft/proof of concept] JTL memory optimizations
carterkozak opened a new pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444
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`
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532463858
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -513,18 +509,15 @@ public void writeString(final StringBuilderFormattable formattable) {
writeNull();
} else {
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- formattable.formatTo(formattableBuffer);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
+ int position = stringBuilder.length();
+ formattable.formatTo(stringBuilder);
+ final int length = stringBuilder.length() - position;
// Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
+ if (length > maxStringLength) {
+ stringBuilder.setLength(position + maxStringLength);
stringBuilder.append(quotedTruncatedStringSuffix);
}
+ StringBuilders.escapeJson(stringBuilder, position);
stringBuilder.append('"');
Review comment:
My comment above regarding `StringBuilders.escapeJson()` applies here too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532612187
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -572,6 +565,10 @@ public void writeString(
}
+ public void writeAsString(Object object) {
Review comment:
> I don't think this should be public. I would rather go with the old approach, where ParameterizedMessage.deepToString() was explicit. This method doesn't provide, IMHO, anything, except saving us to type a couple more characters.
The issue I ran into was using deepToString efficiently requires a (static, to avoid garbage creation) `BiConsumer<StringBuilder, Object>` emitter, which originally I defined in each class that had a 'stringafy' option. I ended up creating a utility function in the writer to make it easier, but it doesn't make much difference either way.
I agree that object->string formatting may be out of scope for the JsonWriter class, and could occur at a higher level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532232183
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/resolver/ReadOnlyStringMapResolver.java
##########
@@ -202,9 +202,7 @@ public void resolve(final LogEvent logEvent, final JsonWriter jsonWriter) {
final ReadOnlyStringMap map = mapAccessor.apply(logEvent);
final Object value = map == null ? null : map.getValue(key);
if (stringified) {
- final String valueString =
- ParameterizedMessage.deepToString(value);
- jsonWriter.writeString(valueString);
Review comment:
When we moved from `Objects.toString(value)` -> `ParameterizedMessage.deepToString(value)` we also changed null handling, where null values previously resulted in the string `"null"` but `ParameterizedMessage.deepToString(` produces a null string in that case, so we would write a json null token.
writeAsString will write the value as `"null"` once again. I should add test coverage separately.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532231594
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java
##########
@@ -302,6 +302,10 @@ public static String deepToString(final Object o) {
return ParameterFormatter.deepToString(o);
}
+ public static void deepToString(final Object o, final StringBuilder buf) {
Review comment:
I think this would make more sense in the StringBuilders utility class, but I put it here for uniformity with the overload.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532469418
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -572,6 +565,10 @@ public void writeString(
}
+ public void writeAsString(Object object) {
Review comment:
This public method allows one to call `jsonWriter.writeAsString("here I break the JSON: \"")`.
1. I don't think this should be public. I would rather go with the old approach, where `ParameterizedMessage.deepToString()` was explicit. This method doesn't provide, IMHO, anything, except saving us to type a couple more characters.
2. All public methods in `JsonWriter` have sufficient user input protection, e.g., null checks, quoting of literals; `writeAsString()` doesn't.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#issuecomment-735789644
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532609807
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -493,18 +492,15 @@ public void writeSeparator() {
final S state) {
Objects.requireNonNull(emitter, "emitter");
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- emitter.accept(formattableBuffer, state);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
- // Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
+ int position = stringBuilder.length();
+ emitter.accept(stringBuilder, state);
+ final int length = stringBuilder.length() - position;
+ if (length > maxStringLength) {
+ // Handle max. string length violating input
+ stringBuilder.setLength(position + maxStringLength);
stringBuilder.append(quotedTruncatedStringSuffix);
}
+ StringBuilders.escapeJson(stringBuilder, position);
Review comment:
> StringBuilders.escapeJson() might add extra characters violating the max. string length constraint.
Are you confident the implementation on release-2.x and master doesn't have a bug in this regard? If max-length is 2 and I write the string `a"`, I'd end up escaping to `a\"`, trimming to `a\`, and appending the truncated string sequence `\u2026` giving me `a\\u2026` where the backslash is escaped rather than producing the `…` character. I think we need to take escape sequences into account when truncating.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#issuecomment-735686828
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:
1. 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()`.
2. 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](https://lists.apache.org/thread.html/r2c20464cf815ff16e6efbf26829db3b3116da16f914e7aa9d6afacc8%40%3Cdev.logging.apache.org%3E). 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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532231934
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -513,18 +509,15 @@ public void writeString(final StringBuilderFormattable formattable) {
writeNull();
} else {
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- formattable.formatTo(formattableBuffer);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
+ int position = stringBuilder.length();
+ formattable.formatTo(stringBuilder);
+ final int length = stringBuilder.length() - position;
// Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
+ if (length > maxStringLength) {
+ stringBuilder.setLength(position + maxStringLength);
stringBuilder.append(quotedTruncatedStringSuffix);
}
+ StringBuilders.escapeJson(stringBuilder, position);
stringBuilder.append('"');
Review comment:
I haven't run all the tests yet, I may have broken something
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532441454
##########
File path: log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java
##########
@@ -302,6 +302,10 @@ public static String deepToString(final Object o) {
return ParameterFormatter.deepToString(o);
}
+ public static void deepToString(final Object o, final StringBuilder buf) {
Review comment:
Fine by me. That said, @remkop, @jvz, and/or @rgoers might have a different opinion on this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532463534
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -493,18 +492,15 @@ public void writeSeparator() {
final S state) {
Objects.requireNonNull(emitter, "emitter");
stringBuilder.append('"');
- formattableBuffer.setLength(0);
- emitter.accept(formattableBuffer, state);
- final int length = formattableBuffer.length();
- // Handle max. string length complying input.
- if (length <= maxStringLength) {
- quoteString(formattableBuffer, 0, length);
- }
- // Handle max. string length violating input.
- else {
- quoteString(formattableBuffer, 0, maxStringLength);
+ int position = stringBuilder.length();
+ emitter.accept(stringBuilder, state);
+ final int length = stringBuilder.length() - position;
+ if (length > maxStringLength) {
+ // Handle max. string length violating input
+ stringBuilder.setLength(position + maxStringLength);
stringBuilder.append(quotedTruncatedStringSuffix);
}
+ StringBuilders.escapeJson(stringBuilder, position);
Review comment:
I fail to see the benefit of this change and have a few objections:
1. I am not inclined to use a separate code base (i.e., `StringBuilders.escapeJson()`) given all the necessary JSON logic, including quoting, is already there. Access to `StringBuilders.escapeJson()` violates the self-containment contract of the class.
2. `StringBuilders.escapeJson()` might add extra characters violating the max. string length constraint.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #444: [draft/proof of concept] JTL memory optimizations
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #444:
URL: https://github.com/apache/logging-log4j2/pull/444#discussion_r532231767
##########
File path: log4j-layout-template-json/src/main/java/org/apache/logging/log4j/layout/template/json/util/JsonWriter.java
##########
@@ -572,6 +565,10 @@ public void writeString(
}
+ public void writeAsString(Object object) {
Review comment:
This could be a `writeString` overload, but I thought it would be easier to understand that it takes potentially non-string values and formats them as strings with a slightly different name. I could go either way on this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org