Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/json performance #1489

Merged
merged 7 commits into from Sep 20, 2019
Merged

Conversation

@dmap
Copy link
Contributor

dmap commented Sep 17, 2019

Improve performance of JsonParser when encoding resources to JSON. There are 3 main improvements:

  • Optimising the search for resources to contain by avoiding repeated traversals of the model
  • Caching the calculation of which composite children need encoding for a given composite
  • Optimise for encoding String primitive data types as these are much more common than the other primitive data types
@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Sep 17, 2019

@dmap this is super cool. Will give a detailed review tomorrow.

Any sense on how much of an improvement it brings?

@dmap

This comment has been minimized.

Copy link
Contributor Author

dmap commented Sep 17, 2019

It all depends on how you measure the improvement, in particular the scale of the improvement depends on the implementation of the java.io.Writer you are encoding to (because things like whether you are compressing the output or not have a large impact on throughput). I'm sure the exact details of the model you are encoding make a difference as well.

Suffice to say that on my test case (encoding large dstu2 DiagnosticReport models) the throughput was approximately doubled.

@dmap

This comment has been minimized.

Copy link
Contributor Author

dmap commented Sep 17, 2019

Apologies for the failing builds... I obviously didn't run them all properly before I created the PR. I'll try to work out why they failed now. The last commit in the PR isn't a performance change, it was just something I noticed in the code that I'm sure was a bug... I'm sure you will have a better idea than me as to whether it really it is or not.

@dmap

This comment has been minimized.

Copy link
Contributor Author

dmap commented Sep 17, 2019

FYI - there is also a PR I created for an improvement in the gson library that improves the performance: google/gson#1576

@dmap

This comment has been minimized.

Copy link
Contributor Author

dmap commented Sep 17, 2019

Sorry, I'm not sure why the builds are failing. When I run locally the test that is currently failing the build works fine, but I have other unrelated tests failing in my local build. My local failures I don't think are because of the changes I have made, they seem to be timezone related (I am based in NZ so UTC+12).

@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Sep 20, 2019

No worries about the failing build. Travis-CI has become very flaky lately, we really need to migrate off of it. The build for your branch passes locally on my machine.

Merging now. Thanks for the awesome PR!

@jamesagnew jamesagnew merged commit c2c07c9 into jamesagnew:master Sep 20, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
jamesagnew added a commit that referenced this pull request Sep 20, 2019
jamesagnew added a commit that referenced this pull request Sep 20, 2019
jamesagnew added a commit that referenced this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.