Skip to content

jsonpb: avoid copying string-valued map-keys #654

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

Merged
merged 1 commit into from
Jul 15, 2018
Merged

Conversation

dhowden
Copy link
Contributor

@dhowden dhowden commented Jul 13, 2018

Encoding large maps with string keys is very expensive. Profiling indicated that the culprit was github.com/golang/protobuf/jsonpb.mapKeys.Less which is used to ensure that the ordering of the map keys are consistent since Go map iteration is random.

By adding a string-specific case we avoid the overhead of going through the fmt.Sprintf calls, which removes a lot of unnecessary allocations.

I suspect that this can be taken further - the requirement of ordered keys does not apply to the JSON rep, so I believe it could be be removed in this case (https://developers.google.com/protocol-buffers/docs/proto3#json). Note: the "text format" rep does require ordering, so maybe this is why the JSON code inherited it...

@puellanivis
Copy link
Collaborator

👍

@dhowden
Copy link
Contributor Author

dhowden commented Jul 15, 2018

Build is passing for go versions 1.10.x and 1.6.x, but failing on 1.x due to:

go build ./protoc-gen-go/testdata/grpc/grpc.pb.go
# golang.org/x/net/http2
../../../golang.org/x/net/http2/go111.go:12:30: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)
../../../golang.org/x/net/http2/go111.go:16:26: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)
../../../golang.org/x/net/http2/go111.go:17:8: trace.WroteHeaderField undefined (type *clientTrace has no field or method WroteHeaderField)
make: *** [test] Error 2

https://travis-ci.org/golang/protobuf/jobs/403362012#L634

@puellanivis
Copy link
Collaborator

This is a known break in the travis CI, and not at all the result of this patch.

@dsnet
Copy link
Member

dsnet commented Jul 15, 2018

Thank you for the PR. @puellanivis is correct that this failure is known (see #650 (comment)).

@dsnet dsnet merged commit 14aad3d into golang:master Jul 15, 2018
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants