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

[WIP] Avoid intermediate values when serializing and deserializing proto3 JSON #670

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Jun 2, 2022

This uses jsontool library to avoid allocating intermediate values when
generating and parsing proto3 JSON strings.

AOT results

Before After Diff
FromProto3JsonString 29639 μs 23353 μs -6286 μs, -21.21%
ToProto3JsonString 35682 μs 26806 μs -8876 μs, -24.88%
FromProto3JsonObject 6637 μs 10486 μs +3849 μs, +57.99%
ToProto3JsonObject 10822 μs 10774 μs -48 μs, -0.44%

JS results

Before After Diff
FromProto3JsonString 34237 μs 63187 μs +28950 μs, +84.56%
ToProto3JsonString 71750 μs 47000 μs -24750 μs, -34.49%
FromProto3JsonObject 17530 μs 35245 μs +17715 μs, +101.06%
ToProto3JsonObject 20343 μs 19627 μs -716 μs, -3.52%

Fixes #662


cc @rakudrama @lrhn

This uses jsontool library to avoid allocating intermediate values when
generating proto3 JSON strings.

Note that the code is not tested for correctness yet, and well-known
types are not supported.

AOT results:

Before:

    ToProto3JsonString(RunTime): 40474.54 us.
    ToProto3JsonString(RunTime): 40340.84 us.
    ToProto3JsonString(RunTime): 40403.9 us.

After:

    ToProto3JsonString(RunTime): 26978.906666666666 us.
    ToProto3JsonString(RunTime): 26891.893333333333 us.
    ToProto3JsonString(RunTime): 26945.04 us.

Diff: -33%

JS results:

Before:

    ToProto3JsonString(RunTime): 71551.72413793103 us.
    ToProto3JsonString(RunTime): 72103.44827586207 us.
    ToProto3JsonString(RunTime): 72035.71428571429 us.

After:

    ToProto3JsonString(RunTime): 45422.22222222222 us.
    ToProto3JsonString(RunTime): 45266.666666666664 us.
    ToProto3JsonString(RunTime): 46813.95348837209 us.

Diff: -36%
@osa1 osa1 requested a review from sigurdm June 2, 2022 12:32
@@ -223,6 +223,13 @@ abstract class GeneratedMessage {
{TypeRegistry typeRegistry = const TypeRegistry.empty()}) =>
_writeToProto3Json(_fieldSet, typeRegistry);

String toProto3JsonString(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider passing an "indent" value

AOT before:

FromProto3JsonString(RunTime): 33019.11475409836 us.
FromProto3JsonString(RunTime): 32874.45901639344 us.
FromProto3JsonString(RunTime): 33174.40983606558 us.

AOT after:

FromProto3JsonString(RunTime): 22567.494382022473 us.
FromProto3JsonString(RunTime): 22654.606741573032 us.
FromProto3JsonString(RunTime): 22849.06818181818 us.

Diff: -31%

JS before:

FromProto3JsonString(RunTime): 34568.96551724138 us.
FromProto3JsonString(RunTime): 35368.42105263158 us.
FromProto3JsonString(RunTime): 35892.857142857145 us.

JS after:

FromProto3JsonString(RunTime): 85875 us.
FromProto3JsonString(RunTime): 83916.66666666667 us.
FromProto3JsonString(RunTime): 82880 us.

Diff: +130%
@osa1 osa1 changed the title [WIP] Avoid intermediate values when serializing proto3 JSON [WIP] Avoid intermediate values when serializing and deserializing proto3 JSON Jun 3, 2022
@@ -223,6 +223,13 @@ abstract class GeneratedMessage {
{TypeRegistry typeRegistry = const TypeRegistry.empty()}) =>
_writeToProto3Json(_fieldSet, typeRegistry);

String toProto3JsonString(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be called writeToProto3Json to maintain consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additions in this PR will be inconsistent unless we want to do breaking changes.

Currently in master branch (and also in the latest release) we have these generators:

  • writeToBuffer
  • writeToCodedBufferWriter
  • writeToJson
  • writeToJsonMap
  • toProto3Json

So the proto3 method is already inconsistent.

Without changing it, we can name the new methods toProto3..., or writeToProto3.... Either way it will be inconsistent.

Should we rename toProto3Json? @sigurdm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh - I see - I introduced that inconsistency - not sure what is the better path forwards.
I guess if we want to be consistent we do:

@Deprecated('Use `writeToProto3JsonMap`');
toProto3Json() => writeToProto3JsonMap();

writeToProto3JsonMap();
writeToProto3Json();

If we prefer to avoid churn over consistency we keep it as toProto3JsonString()....

I guess it's up to your judgement.

@osa1 osa1 mentioned this pull request Jun 10, 2022
@osa1
Copy link
Member Author

osa1 commented Jun 21, 2022

Looking at the benchmark results again, we have 3 regressions:

  1. AOT JSON object to message
  2. JS JSON object to message
  3. JS JSON string to message

If we keep the current JSON object parser in mergeFromProto3Json and only use jsontool when parsing JSON strings that will fix (3) as well as we can do mergeFromProto3Json(decodeJson(...)) when targeting JS.

This means extra ~300 lines of code to maintain, per JS format..

When testing, we can avoid duplicating test code by running the tests on JS as well. Since tests are ordinary Dart programs we can compile them to JS with dart compile js ... and run with v8/d8. To keep it easier to compile and run, we could run all tests in a file all_tests.dart (which we used to do a while ago, I think because of how dart test worked at the time) and just compile and run that in CI, instead of each test file individually.

@osa1
Copy link
Member Author

osa1 commented Jun 21, 2022

I'm splitting this into two PRs, for generating JSON and parsing JSON. #683 updates JSON generators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid intermediate JSON objects when serializing and deserializing
2 participants