-
Notifications
You must be signed in to change notification settings - Fork 185
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
base: master
Are you sure you want to change the base?
Conversation
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. |
@rakudrama I can't add you as a reviewer, could you take a look please? |
@osa1 LGTM |
return buf.toString(); | ||
} | ||
|
||
void writeToJsonSink(JsonSink jsonSink) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 AnyMixin
s 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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jsontool package is a third-party package which is not supported by the Dart team. |
We should add Secondly, we can keep the current |
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
JS runtime
Benchmark binary sizes
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%.