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

Avoid intermediate JSON object allocations when generating JSON strings #683

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Jun 21, 2022

Use jsontool library to avoid generating intermediate JSON objects when
generating JSON strings from messages.

Original proto3 code moved from #670. Generator for other JSON format is
updated to use jsontool as well.

Splits JSON parsers and generators into separate files.

AOT runtime

Before After Diff
ToJsonObject 10304 μs 11175 μs +871 μs, +8.45%
ToJsonString 35030 μs 25946 μs -9084 μs, -25.93%
ToProto3JsonObject 10812 μs 10824 μs +12 μs, +0.11%
ToProto3JsonString 35906 μs 26758 μs -9148 μs, -25.48%

JS runtime

Before After Diff
ToJsonObject 19637 μs 23313 μs +3676 μs, +18.72%
ToJsonString 71535 μs 45477 μs -26058 μs, -36.43%
ToProto3JsonObject 22318 μs 20323 μs -1995 μs, -8.94%
ToProto3JsonString 71642 μs 50000 μs -21642 μs, -30.21%

Benchmark binary sizes

Before After Diff
AOT 6,990,992 bytes 7,010,576 bytes +19,584 bytes, +0.28%
JS 338,060 bytes 339,829 bytes +1,769 bytes +0.52%

Internal

jsontool library added to internal in cl/466006106
This PR ported to internal in cl/473230561

A large JS release binary size reduced by 0.22%.

@osa1
Copy link
Member Author

osa1 commented Jul 14, 2022

I benchmarked this PR on Golem: https://golem.corp.goog/Revision?repository=protobuf&revision=84839&patch=17197

This PR has no changes in binary serialization or deserialization (only changes JSON), but Golem reports 5% increase in a binary serialization benchmark (to_binary.dart in benchmarks/).

It seems like Golem results can also be quite noisy and cannot be trusted.

@osa1 osa1 marked this pull request as ready for review September 13, 2022 13:58
@osa1
Copy link
Member Author

osa1 commented Sep 13, 2022

@rakudrama I can't add you as a reviewer, could you take a look please?

@rakudrama
Copy link
Contributor

@osa1 LGTM

return buf.toString();
}

void writeToJsonSink(JsonSink jsonSink) {
Copy link
Collaborator

@sigurdm sigurdm Sep 15, 2022

Choose a reason for hiding this comment

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

Do we need to expose this?

Not that I expect the interface to change anytime soon, but exposing api from another package is always extra liability (we cannot change without going through jsontool), and as such we should prefer to keep usage internal.

On the other hand we are more or less marrying to the jsontool package here, so it might not matter much.

Copy link
Member Author

@osa1 osa1 Sep 15, 2022

Choose a reason for hiding this comment

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

Good point re: exposing API from another library. No one is requesting this at the moment, and users start using the sink API it will be difficult to migrate to another library later, or maybe drop jsontool entirely.

OTOH if needed we can always add it later without breakage (other than messages with field name write_to_json_sink).

I'll remove sink methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed writeToJsonSink, but we can't remove toProto3JsonSink because it's called from AnyMixins toProtoJsonHelper, which now takes a sink argument as well to allow reuse from entry points toProto3JsonString and toProto3JsonObject. I think the best we can do is hiding toProto3JsonSink in documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely a liability to expose a third-party package's API in your API. If they change the API, it affects your API too, which means you might need a tight version constraint.

If the method can be hidden in documentation, it's not clear that users ever need to call it. Could it be made private, and only accessible by calling a special static helper function, like:

 .... 
 dynamic _toProto3JsonObject(...) ...
}

dynamic toProto3JsonObject(YourClass o) => o._toProto3JsonObject();

and then don't export that function in the public API (hide it), just rely on it in AnyMixin.

Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Code LGTM - only my little question if we should expose the method taking a JSON-sink or not...

@mit-mit @lrhn
Before we can roll this into the sdk we will need jsontool to move to the tools.dart.dev publisher.

@lrhn
Copy link
Contributor

lrhn commented Sep 19, 2022

The jsontool package is a third-party package which is not supported by the Dart team.
Would you consider forking or vendoring it?

@osa1
Copy link
Member Author

osa1 commented Oct 27, 2023

We should add GeneratedMessage members to generate encoded JSON, ready for writing to a socket.

Secondly, we can keep the current toJsonObject implementation as it's faster on both AOT and JS.

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.

4 participants