Clarify semantics around Jackson's ByteBufferFeeder #2425

daschl · 2022-11-17T17:43:44Z

Motivation:

In 2.14.0, Jackson added support for feeding ByteBuffers directly
into the streaming parser engine (where previously only byte arrays
would be supported). This changeset adds a benchmark and provides
rationale why the decision has been made to stick with the current
code.

Modifications:

The benchmark numbers show that there is no substantial benefit
in either completely switching over to the ByteBufferFeeder or
decide which one to use when the first buffer arrives. In some
scenarios it is even slower.

As a drive-by fix, the feeder is now properly closed when the
operator terminates successfully or because of an error.

idelpivnitskiy · 2022-11-18T18:35:19Z

Hey @daschl, thank you for the contribution! I will have a closer look soon.

From the build logs:

Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:19:1: Import statement for 'io.servicetalk.buffer.api.Buffer' is in the wrong order. Should be in the 'SPECIAL_IMPORTS' group, expecting group 'STANDARD_JAVA_PACKAGE' on this line. [CustomImportOrder]

Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:20:1: Import statement for 'io.servicetalk.buffer.netty.BufferAllocators' is in the wrong order. Should be in the 'SPECIAL_IMPORTS' group, expecting group 'STANDARD_JAVA_PACKAGE' on this line. [CustomImportOrder]
> Task :servicetalk-benchmarks:checkstyleJmh
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:61:46: '{' is not followed by whitespace. [WhitespaceAround]
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:61:47: '}' is not preceded with whitespace. [WhitespaceAround]
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:127: An empty line at the end of the code block [EmptyLineBeforeBlockClose]

> Task :servicetalk-benchmarks:checkstyleJmh FAILED

We have some checkstyle rules for the imports. Those rules will be installed automatically for your project in Intellij IDEA if you use ./gradlew idea to generate the project structure instead of opening the source code by Intellij IDEA directly. For more information, see Contributor setup.

daschl · 2022-11-18T18:37:13Z

Hey @daschl, thank you for the contribution! I will have a closer look soon.

From the build logs:

Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:19:1: Import statement for 'io.servicetalk.buffer.api.Buffer' is in the wrong order. Should be in the 'SPECIAL_IMPORTS' group, expecting group 'STANDARD_JAVA_PACKAGE' on this line. [CustomImportOrder]

Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:20:1: Import statement for 'io.servicetalk.buffer.netty.BufferAllocators' is in the wrong order. Should be in the 'SPECIAL_IMPORTS' group, expecting group 'STANDARD_JAVA_PACKAGE' on this line. [CustomImportOrder]
> Task :servicetalk-benchmarks:checkstyleJmh
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:61:46: '{' is not followed by whitespace. [WhitespaceAround]
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:61:47: '}' is not preceded with whitespace. [WhitespaceAround]
Error: eckstyle] [ERROR] /home/runner/work/servicetalk/servicetalk/servicetalk-benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java:127: An empty line at the end of the code block [EmptyLineBeforeBlockClose]

> Task :servicetalk-benchmarks:checkstyleJmh FAILED

We have some checkstyle rules for the imports. Those rules will be installed automatically for your project in Intellij IDEA if you use ./gradlew idea to generate the project structure instead of opening a project by Intellij IDEA. For more information, see Contributor setup.

Thanks for sharing - I'll do the checkstyle cleanup alongside other feedback once you've had a chance to take a closer look if that's alright.

idelpivnitskiy · 2022-11-18T18:38:14Z

Absolutely, sent an earlier message just to prevent you from spending time to figure out what is the correct order 😄

idelpivnitskiy · 2022-11-19T02:48:26Z

...benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java

+ * JacksonStreamingSerializerBenchmark.deserializeMidBackedByArray     thrpt    5   527170,664 ± 107330,448  ops/s
+ * JacksonStreamingSerializerBenchmark.deserializeMidBackedByDirect    thrpt    5   420306,213 ±  78607,263  ops/s
+ * JacksonStreamingSerializerBenchmark.deserializeSmallBackedByArray   thrpt    5  1307308,103 ± 128131,629  ops/s
+ * JacksonStreamingSerializerBenchmark.deserializeSmallBackedByDirect  thrpt    5   674688,550 ± 108705,165  ops/s


Thanks for adding a benchmark! In the PR description or comments can you please add numbers before/after the change for comparison?

idelpivnitskiy · 2022-11-19T02:52:02Z

...benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java

+    "est.\\r\\n\",\"registered\":\"2022-01-01T03:44:39 -01:00\",\"latitude\":61.061004,\"longitude\":38.544479," +
+    "\"tags\":[\"qui\",\"nulla\",\"cillum\",\"dolor\",\"deserunt\",\"amet\",\"magna\"],\"friends\":[{\"id\":0," +
+    "\"name\":\"Gibson Benson\"},{\"id\":1,\"name\":\"Marshall Atkins\"},{\"id\":2,\"name\":\"Gabriel Clay\"}," +
+    "{\"id\":3,\"name\":\"Aguilar\"},{\"id\":4,\"name\":\"Bauer\"}]}";


Are these data generated? trying to make sure we do not hold anyone's info :)

idelpivnitskiy · 2022-11-19T02:53:23Z

...benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java

+    "\"picture\":\"https://placehold.it/32x32\"}";
+
+  /**
+   * Represents a 1000-byte JSON object (encoded).


Don't hesitate to add really large files. That will help to see the difference better. Consider 256B, 2Kb, 16Kb JSON files.

add really large files -> if we go this route consider generating the content so we don't have to check it in to git :)

idelpivnitskiy · 2022-11-19T02:56:04Z

...benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java

+  private static Map<String, Object> deserialize(final Buffer fromBuffer) throws Exception {
+    return JACKSON
+      .streamingSerializerDeserializer(MAP_TYPE_REFERENCE)
+      .deserialize(from(fromBuffer), BufferAllocators.DEFAULT_ALLOCATOR)


While it uses streaming API, effectively the entire payload is aggregated. To simulate a streaming use-case, consider splitting JSON into multiple chunks (at least 10 but could be up to 100). It will help us to see how Jackson manages its internal state during streaming data flow.

idelpivnitskiy · 2022-11-19T03:13:10Z

...etalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonStreamingSerializer.java

        }

-        private static final class ByteArrayDeserializeSubscriber<T> extends DeserializeSubscriber<T> {
-            private final ByteArrayFeeder feeder;
+        private static class DeserializeSubscriber<T> implements Subscriber<Buffer> {


Consider making it final

idelpivnitskiy · 2022-11-19T03:32:05Z

...etalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonStreamingSerializer.java

+                    isByteArrayParser = true;
+                } else {
+                    parser = reader.getFactory().createNonBlockingByteBufferParser();
+                    isByteArrayParser = false;


If ByteBufferFeeder is available now, I wonder if we can simplify the control flow to always use it and forget about ByteArrayParser. The ByteBuffer can be backed by direct memory or heap memory. I would expect it to work with any type of ByteBuffer. WDYT?

For backwards compatibility (e.g. if users link an older version of Jackson) we may have to still fallback to ByteArray. This is assuming perf is roughly equivalent (I expect some additional object allocation overhead if byte[] needs to be wrapped in ByteBuffer, and I'm not sure how efficient our toNioBuffer() is ..).

idelpivnitskiy · 2022-11-19T03:32:40Z

...etalk-data-jackson/src/main/java/io/servicetalk/data/jackson/JacksonStreamingSerializer.java

                subscriber.onError(t);
            }

            @Override
            public final void onComplete() {
+                if (feeder != null) {
+                    feeder.endOfInput();


Good catch!

daschl · 2022-12-01T16:47:25Z

@idelpivnitskiy changeset polished up per review - here are the before and after benchmarks:

PR

Benchmark                                                            Mode  Cnt        Score      Error  Units
JacksonStreamingSerializerBenchmark.deserializeLargeBackedByArray   thrpt   10   102853,701 ±  636,206  ops/s
JacksonStreamingSerializerBenchmark.deserializeLargeBackedByDirect  thrpt   10   101887,661 ±  318,581  ops/s
JacksonStreamingSerializerBenchmark.deserializeMidBackedByArray     thrpt   10   256065,426 ±  457,192  ops/s
JacksonStreamingSerializerBenchmark.deserializeMidBackedByDirect    thrpt   10   237096,527 ±  716,018  ops/s
JacksonStreamingSerializerBenchmark.deserializeSmallBackedByArray   thrpt   10  1129949,785 ± 1387,879  ops/s
JacksonStreamingSerializerBenchmark.deserializeSmallBackedByDirect  thrpt   10  1026503,131 ± 2218,478  ops/s

stable (0.42.22)

Benchmark                                                            Mode  Cnt        Score      Error  Units
JacksonStreamingSerializerBenchmark.deserializeLargeBackedByArray   thrpt   10   107283,809 ±  435,752  ops/s
JacksonStreamingSerializerBenchmark.deserializeLargeBackedByDirect  thrpt   10   102547,028 ±  278,600  ops/s
JacksonStreamingSerializerBenchmark.deserializeMidBackedByArray     thrpt   10   293569,073 ± 1079,336  ops/s
JacksonStreamingSerializerBenchmark.deserializeMidBackedByDirect    thrpt   10   232366,394 ±  568,238  ops/s
JacksonStreamingSerializerBenchmark.deserializeSmallBackedByArray   thrpt   10  1108666,880 ± 3319,926  ops/s
JacksonStreamingSerializerBenchmark.deserializeSmallBackedByDirect  thrpt   10  1132111,074 ± 1764,877  ops/s

They are in the same ballpark, only deserializeMidBackedByArray on stable seems to perform a bit better but I'm not sure if that's a big enough difference to keep investigating given that smaller and larger seem to be identical.

idelpivnitskiy

After you adjust the memory type, would be nice to run 3 options: pre-existing behavior with Jackson 2.14, this PR, a version that always uses new ByteBufferFeeder and toNioBuffer().

idelpivnitskiy · 2022-12-02T18:23:29Z

...benchmarks/src/jmh/java/io/servicetalk/data/jackson/JacksonStreamingSerializerBenchmark.java

+    private static final Buffer LARGE_JSON_HEAP =
+            BufferAllocators.PREFER_HEAP_ALLOCATOR.fromUtf8(generateJson(1_000_000));
+    private static final Buffer LARGE_JSON_DIRECT =
+            BufferAllocators.PREFER_DIRECT_ALLOCATOR.wrap(LARGE_JSON_HEAP.toNioBuffer());


The numbers are not different much between heap/direct because you always use heap buffers :)
Have a look inside wrap method.
In order to get a direct memory, you need to use either PREFER_DIRECT_ALLOCATOR.fromUtf8 method or PREFER_DIRECT_ALLOCATOR.newBuffer(...) and then copy content from LARGE_JSON_HEAP.
When you adjust, consider using the same content (cache generateJson result or copy from heap to direct).

Motivation: In 2.14.0, Jackson added support for feeding ByteBuffers directly into the streaming parser engine (where previously only byte arrays would be supported). This changeset adds a benchmark and provides rationale why the decision has been made to stick with the current code. Modifications: The benchmark numbers show that there is no substantial benefit in either completely switching over to the ByteBufferFeeder or decide which one to use when the first buffer arrives. In some scenarios it is even slower. As a drive-by fix, the feeder is now properly closed when the operator terminates successfully or because of an error.

idelpivnitskiy

Thanks for looking into that, was a nice attempt. Hope Jackson improves in the future 🤞

idelpivnitskiy self-requested a review November 18, 2022 16:40

idelpivnitskiy reviewed Nov 19, 2022

View reviewed changes

idelpivnitskiy assigned daschl Nov 30, 2022

daschl force-pushed the issue-1711 branch 3 times, most recently from ce13c87 to 7e2bbbe Compare December 1, 2022 15:56

idelpivnitskiy reviewed Dec 2, 2022

View reviewed changes

daschl force-pushed the issue-1711 branch from 7e2bbbe to 8d4c16f Compare December 3, 2022 00:33

daschl changed the title ~~Add support for Jackson's new ByteBufferFeeder (#1711)~~ Clarify semantics around Jackson's ByteBufferFeeder Dec 3, 2022

Fix checkstyle issues

8ffe9e8

daschl requested review from idelpivnitskiy and Scottmitch December 3, 2022 00:37

Do not rely on java 17 APIs

fe086cb

idelpivnitskiy approved these changes Dec 3, 2022

View reviewed changes

fix failing testcase

27a5015

daschl merged commit f3636fb into apple:main Dec 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify semantics around Jackson's ByteBufferFeeder #2425

Clarify semantics around Jackson's ByteBufferFeeder #2425

daschl commented Nov 17, 2022 •

edited

Loading

idelpivnitskiy commented Nov 18, 2022 •

edited

Loading

daschl commented Nov 18, 2022

idelpivnitskiy commented Nov 18, 2022

idelpivnitskiy Nov 19, 2022

idelpivnitskiy Nov 19, 2022 •

edited

Loading

idelpivnitskiy Nov 19, 2022

Scottmitch Nov 22, 2022

idelpivnitskiy Nov 19, 2022

idelpivnitskiy Nov 19, 2022

idelpivnitskiy Nov 19, 2022

Scottmitch Nov 22, 2022

idelpivnitskiy Nov 19, 2022

daschl commented Dec 1, 2022 •

edited

Loading

idelpivnitskiy left a comment

idelpivnitskiy Dec 2, 2022 •

edited

Loading

idelpivnitskiy left a comment

Clarify semantics around Jackson's ByteBufferFeeder #2425

Clarify semantics around Jackson's ByteBufferFeeder #2425

Conversation

daschl commented Nov 17, 2022 • edited Loading

idelpivnitskiy commented Nov 18, 2022 • edited Loading

daschl commented Nov 18, 2022

idelpivnitskiy commented Nov 18, 2022

Choose a reason for hiding this comment

idelpivnitskiy Nov 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschl commented Dec 1, 2022 • edited Loading

idelpivnitskiy left a comment

Choose a reason for hiding this comment

idelpivnitskiy Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

idelpivnitskiy left a comment

Choose a reason for hiding this comment

daschl commented Nov 17, 2022 •

edited

Loading

idelpivnitskiy commented Nov 18, 2022 •

edited

Loading

idelpivnitskiy Nov 19, 2022 •

edited

Loading

daschl commented Dec 1, 2022 •

edited

Loading

idelpivnitskiy Dec 2, 2022 •

edited

Loading