Avoid intermediate JSON object allocations when generating JSON strings#683
Avoid intermediate JSON object allocations when generating JSON strings#683osa1 wants to merge 23 commits intogoogle:masterfrom
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.
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.
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.
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.
There was a problem hiding this comment.
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.
|
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%.