Performance improvements for JacksonConfigParser #218

loganlinn · 2018-09-24T19:30:52Z

This addresses some poor use of Jackson when deserializing JSON to
ProjectConfig object and improves parse performance by about 15x-40x
based on some benchmarks.

The custom deserializers were creating new ObjectMapper instances on
each deserialize() invocation. Very wasteful.

Eliminates several cases of unecessary round trips of reading JSON
string to JSON tree, then back to JSON string to parse a child node.

We can eliminate the GroupJacksonDeserializer completly and rely on
standard Jackson databind annotations.

Deserialize Audience conditions using JsonNode interface to eliminate
intermediate mapping to List and Map.

Adds JacksonConfigParserBenchmark (fixes Gradle JMH setup).

Benchmark results:

Before:

Benchmark                             Mode  Cnt     Score     Error  Units
JacksonConfigParserBenchmark.parseV2  avgt   40  1668.109 ± 178.586  us/op
JacksonConfigParserBenchmark.parseV3  avgt   40  1574.464 ±  25.816  us/op
JacksonConfigParserBenchmark.parseV4  avgt   40  2166.621 ±  91.664  us/op

After:

Benchmark                             Mode  Cnt    Score   Error  Units
JacksonConfigParserBenchmark.parseV2  avgt   40   41.292 ± 1.219  us/op
JacksonConfigParserBenchmark.parseV3  avgt   40   54.360 ± 0.790  us/op
JacksonConfigParserBenchmark.parseV4  avgt   40  128.589 ± 3.728  us/op

coveralls · 2018-09-24T20:54:49Z

Pull Request Test Coverage Report for Build 684

69 of 74 (93.24%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 89.199%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/parser/JacksonHelpers.java	8	9	88.89%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	7	9	77.78%
core-api/src/main/java/com/optimizely/ab/config/parser/AudienceJacksonDeserializer.java	16	18	88.89%

Totals
Change from base Build 682:	-0.2%
Covered Lines:	2494
Relevant Lines:	2796

💛 - Coveralls

This addresses some poor use of Jackson when deserializing JSON to `ProjectConfig` object and improves parse performance by about 15x-40x based on some benchmarks. The custom deserializers were creating new `ObjectMapper` instances on each `deserialize()` invocation. Very wasteful. Eliminates several cases of unecessary round trips of reading JSON string to JSON tree, then back to JSON string to parse a child node. We can eliminate the `GroupJacksonDeserializer` completly and rely on standard Jackson databind annotations. Deserialize Audience conditions using `JsonNode` interface to eliminate intermediate mapping to `List` and `Map`. Adds `JacksonConfigParserBenchmark` (fixes Gradle JMH setup). Benchmark results: Before: Benchmark Mode Cnt Score Error Units JacksonConfigParserBenchmark.parseV2 avgt 40 1668.109 ± 178.586 us/op JacksonConfigParserBenchmark.parseV3 avgt 40 1574.464 ± 25.816 us/op JacksonConfigParserBenchmark.parseV4 avgt 40 2166.621 ± 91.664 us/op After: Benchmark Mode Cnt Score Error Units JacksonConfigParserBenchmark.parseV2 avgt 40 41.292 ± 1.219 us/op JacksonConfigParserBenchmark.parseV3 avgt 40 54.360 ± 0.790 us/op JacksonConfigParserBenchmark.parseV4 avgt 40 128.589 ± 3.728 us/op

mikeproeng37

Thanks a lot for doing this! Especially for adding the benchmark test for showing the results of the improvement =)

Looks good to me. Just sending back so we can remove those two deserializer classes and update the license headers in the files.

mikeproeng37 · 2018-09-26T20:28:22Z

core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java

    @Override
    public ProjectConfig deserialize(JsonParser parser, DeserializationContext context) throws IOException {
-        ObjectMapper mapper = new ObjectMapper();
-        SimpleModule module = new SimpleModule();
-        module.addDeserializer(Audience.class, new AudienceJacksonDeserializer());


Nice catch! Looks like we won't be needing these two classes anymore. Do you mind removing them as well?

These are still being used -- I moved these into a a proper Module class of their own. See ProjectConfigModule. We can definitely get rid of them with a little more work, but tried to keep the moving parts to a minimum and focus on perf in this PR.

mikeproeng37 · 2018-09-26T20:28:51Z

.editorconfig

@@ -0,0 +1,9 @@
+# EditorConfig is awesome: http://EditorConfig.org


This is pretty cool! Will start using it in our projects :)

mikeproeng37 · 2018-09-26T20:30:23Z

core-api/src/jmh/java/com/optimizely/ab/config/parser/JacksonConfigParserBenchmark.java

@@ -0,0 +1,44 @@
+package com.optimizely.ab.config.parser;


Please add the license header to this file. You only need to include the year 2018 because it was created this year.

mikeproeng37 · 2018-09-26T20:30:39Z

core-api/src/main/java/com/optimizely/ab/config/Experiment.java

-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
+import com.fasterxml.jackson.annotation.*;


Please update the license year to 2016-2018 for this and all the files you touched.

vraja2

great improvement!

loganlinn

I've updated the copyright headers for files changed in this PR and merged in latest changes from master. @mikeng13 could you take another look?

loganlinn · 2018-10-05T18:52:02Z

core-api/src/main/java/com/optimizely/ab/config/parser/ProjectConfigJacksonDeserializer.java

    @Override
    public ProjectConfig deserialize(JsonParser parser, DeserializationContext context) throws IOException {
-        ObjectMapper mapper = new ObjectMapper();
-        SimpleModule module = new SimpleModule();
-        module.addDeserializer(Audience.class, new AudienceJacksonDeserializer());


These are still being used -- I moved these into a a proper Module class of their own. See ProjectConfigModule. We can definitely get rid of them with a little more work, but tried to keep the moving parts to a minimum and focus on perf in this PR.

mikeproeng37

lgtm

mikeproeng37 · 2018-10-09T17:27:07Z

build

mikeproeng37 · 2018-10-09T17:27:55Z

@loganlinn looks like there are some lint errors from the Travis build:

Execution failed for task ':core-api:findbugsMain'.
> FindBugs rule violations were found. See the report at: file:///home/travis/build/optimizely/java-sdk/core-api/build/reports/findbugs/main.html```

loganlinn · 2018-10-09T21:32:16Z

@mikeng13 all fixed up

mikeproeng37 · 2018-10-09T22:25:53Z

Thanks! I looked at the Travis builds and they are passing now. HOwever they aren't reporting back to Github. I will merge anyway since all required tests pass. Thanks again for the contribution!

loganlinn requested a review from vraja2 September 24, 2018 19:30

loganlinn force-pushed the llinn/jackson-perf branch from 6d08b4e to fbdf1b5 Compare September 24, 2018 21:10

loganlinn requested a review from mikeproeng37 September 24, 2018 21:10

mikeproeng37 suggested changes Sep 26, 2018

View reviewed changes

vraja2 approved these changes Sep 26, 2018

View reviewed changes

loganlinn added 4 commits October 5, 2018 12:02

Copyright headers

6088dc0

Merge remote-tracking branch 'origin/master' into llinn/jackson-perf

32729d3

Polymorphic UserAttribute value parsing

2689e8f

Merge branch 'master' into llinn/jackson-perf

a926d5c

loganlinn commented Oct 8, 2018

View reviewed changes

mikeproeng37 approved these changes Oct 8, 2018

View reviewed changes

loganlinn added 2 commits October 9, 2018 14:31

Fix FindBugs warnings

d351774

Merge branch 'master' into llinn/jackson-perf

d72d7b3

mikeproeng37 merged commit 6366bad into master Oct 9, 2018

loganlinn deleted the llinn/jackson-perf branch October 9, 2018 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Performance improvements for JacksonConfigParser #218

Performance improvements for JacksonConfigParser #218

Uh oh!

loganlinn commented Sep 24, 2018

Uh oh!

coveralls commented Sep 24, 2018 •

edited

Loading

Uh oh!

mikeproeng37 left a comment

Uh oh!

mikeproeng37 Sep 26, 2018

Uh oh!

loganlinn Oct 5, 2018

Uh oh!

mikeproeng37 Sep 26, 2018

Uh oh!

mikeproeng37 Sep 26, 2018

Uh oh!

mikeproeng37 Sep 26, 2018

Uh oh!

vraja2 left a comment

Uh oh!

loganlinn left a comment

Uh oh!

loganlinn Oct 5, 2018

Uh oh!

mikeproeng37 left a comment

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

loganlinn commented Oct 9, 2018

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

Uh oh!

		@@ -0,0 +1,9 @@
		# EditorConfig is awesome: http://EditorConfig.org

Performance improvements for JacksonConfigParser #218

Performance improvements for JacksonConfigParser #218

Uh oh!

Conversation

loganlinn commented Sep 24, 2018

Uh oh!

coveralls commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 684

💛 - Coveralls

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

loganlinn Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 Sep 26, 2018

Choose a reason for hiding this comment

Uh oh!

vraja2 left a comment

Choose a reason for hiding this comment

Uh oh!

loganlinn left a comment

Choose a reason for hiding this comment

Uh oh!

loganlinn Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

loganlinn commented Oct 9, 2018

Uh oh!

mikeproeng37 commented Oct 9, 2018

Uh oh!

Uh oh!

coveralls commented Sep 24, 2018 •

edited

Loading