golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.74k stars 1.58k forks source link

encoding/protojson: poor serialization performance #1285

Open tonybase opened 3 years ago

tonybase commented 3 years ago

https://github.com/tonybase/benchmark/tree/main/benchmark-protobuf-json:

➜  benchmark-protobuf-json git:(main) go test -bench=. -benchmem
goos: darwin
goarch: amd64
pkg: github.com/tonybase/benchmark/benchmark-protobuf-json
cpu: Intel(R) Core(TM) i7-8557U CPU @ 1.70GHz
BenchmarkOrderProto3Marshal-8        3230240           352.5 ns/op       112 B/op          1 allocs/op
BenchmarkOrderJSONMarshal-8          1411366           858.9 ns/op       240 B/op          1 allocs/op
BenchmarkOrderJSONPBMarshal-8         259860          4432 ns/op        1016 B/op         29 allocs/op
BenchmarkOrderProto3Unmarshal-8      1879772           630.8 ns/op       368 B/op         11 allocs/op
BenchmarkOrderJSONUnmarshal-8         288832          3992 ns/op         696 B/op         20 allocs/op
BenchmarkOrderJSONPBUnmarshal-8       181722          6217 ns/op        1088 B/op         56 allocs/op
PASS
ok      github.com/tonybase/benchmark/benchmark-protobuf-json   9.430s

It doesn't seem to optimize performance very well, and Encoder doesn't use a similar standard library EncodeStatePool to reduce object allocation

dsnet commented 3 years ago

The protojson package uses its own implementation of JSON since it needs to comply with a stricter version of JSON (RFC 7493) than what encoding/json supports (RFC 7159). The internal implementation does not benefit from years of optimizations that have been added to the standard library. There's some work going on to investigate major changes to the encoding/json package which may allow us to have native RFC 7159 support, in which case we will delete the internal library and use the standard one instead (and benefit from its optimizations).

tonybase commented 3 years ago

The main reason that encoding/json is not used is because google/protobuf/duration.proto, int64(string) and oneof marshal are inconsistent with the standard library JSON and there is no good solution.

https://developers.google.com/protocol-buffers/docs/proto3#json

tonybase commented 3 years ago

I want to use encoding/json. Can I implement MarshalJSON and UnmarshalJSON interfaces for known types?

https://github.com/protocolbuffers/protobuf-go/tree/master/types/known

dsnet commented 3 years ago

The main reason that encoding/json is not used is because google/protobuf/duration.proto, int64(string) and oneof marshal are inconsistent with the standard library JSON and there is no good solution.

That's not true. There are many reasons why encoding/json is not used including the representation of oneofs, well-known types, and so forth.

related: protocolbuffers/protobuf#8331

Let's keep this topic about improving performance. Trying to serialize int64 as a JSON number is an unrelated concern.

ash2k commented 3 years ago

Another potential performance optimization is to allow to disable field sorting. Any/random order is fine in most situations.

tonybase commented 2 years ago

Are there any current plans for this optimization?

schattian commented 2 years ago

Hi, afaik the issue tracker continues to be this one both for this and the newer protobuf-go repo, let me know if that's not the case.

I've submitted a PR to improve marshaling performance at https://go-review.googlesource.com/c/protobuf/+/373356

name                        old time/op    new time/op    delta
Marshal_ScalarsUnordered-8    3.49µs ± 8%    2.06µs ± 1%  -40.84%  (p=0.000 n=29+27)
Marshal_Scalars-8             4.18µs ± 7%    2.80µs ± 1%  -32.94%  (p=0.000 n=29+30)

name                        old alloc/op   new alloc/op   delta
Marshal_ScalarsUnordered-8    1.47kB ± 0%    0.14kB ± 0%  -90.76%  (p=0.000 n=30+30)
Marshal_Scalars-8             1.63kB ± 0%    0.30kB ± 0%  -81.87%  (p=0.000 n=30+30)

name                        old allocs/op  new allocs/op  delta
Marshal_ScalarsUnordered-8      29.0 ± 0%       4.0 ± 0%  -86.21%  (p=0.000 n=30+30)
Marshal_Scalars-8               34.0 ± 0%       9.0 ± 0%  -73.53%  (p=0.000 n=30+30)

Boring details: I've added the 'Unordered' marshalopt on the old impl to sample unordered and ordered benchmarks.

puellanivis commented 2 years ago

With benchstat try running with -count=10 to get ten samples each, and then the delta will provide you with details about how much better/worse the metric is, and the confidence of that value. You’ll notice all of these have a p-value of 1, which is not particularly convincing on its own.

schattian commented 2 years ago

updated

schattian commented 2 years ago

going back to this in a bit, on vacation rn