-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Implement json.Marshal just for strings #4979
Conversation
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.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ashish-goswami and @pawanrawal)
query/outputnode.go, line 232 at r1 (raw file):
switch v.Tid { case types.StringID, types.DefaultID: return stringJsonMarshal(v.Value.(string)), nil
switch the type. Use stringJsonMarshal, otherwise json.Marshal.
query/table.go, line 7 at r1 (raw file):
import "unicode/utf8"
Can you mention which file did we copy and from where.
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.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ashish-goswami)
We are using json.Marshal() in our code to convert strings to bytes. This function is very generic function and can be used to marshal any type. For our use case, most of the time, this is called for string type. When we dig deeper into the function's code, most of the time is spent on finding the right encoder function for type. In our case stringEncoder. This PR takes away the code of encodeState.string() and defines internally, which is just called for string type. Here are the benchmarks: go test -v -run ^$ -bench BenchmarkJsonMarshal -benchtime=20s [Decoder]: Using assembly version of decoder goos: linux goarch: amd64 pkg: github.com/dgraph-io/dgraph/query BenchmarkJsonMarshal/STDJsonMarshal-largestring-16 11113017 2128 ns/op BenchmarkJsonMarshal/stringJsonMarshal-largestring-16 12233304 1957 ns/op BenchmarkJsonMarshal/STDJsonMarshal-smallstring-16 100000000 252 ns/op BenchmarkJsonMarshal/stringJsonMarshal-smallstring-16 271762213 87.7 ns/op BenchmarkJsonMarshal/STDJsonMarshal-specialchars-16 37255737 636 ns/op BenchmarkJsonMarshal/stringJsonMarshal-specialchars-16 52765609 463 ns/op PASS ok github.com/dgraph-io/dgraph/query 159.218s
We are using json.Marshal() in our code to convert strings to bytes. This function is very generic function and can be used to marshal any type. For our use case, most of the time, this is called for string type. When we dig deeper into the function's code, most of the time is spent on finding the right encoder function for type. In our case stringEncoder. This PR takes away the code of encodeState.string() and defines internally, which is just called for string type. Here are the benchmarks: go test -v -run ^$ -bench BenchmarkJsonMarshal -benchtime=20s [Decoder]: Using assembly version of decoder goos: linux goarch: amd64 pkg: github.com/dgraph-io/dgraph/query BenchmarkJsonMarshal/STDJsonMarshal-largestring-16 11113017 2128 ns/op BenchmarkJsonMarshal/stringJsonMarshal-largestring-16 12233304 1957 ns/op BenchmarkJsonMarshal/STDJsonMarshal-smallstring-16 100000000 252 ns/op BenchmarkJsonMarshal/stringJsonMarshal-smallstring-16 271762213 87.7 ns/op BenchmarkJsonMarshal/STDJsonMarshal-specialchars-16 37255737 636 ns/op BenchmarkJsonMarshal/stringJsonMarshal-specialchars-16 52765609 463 ns/op PASS ok github.com/dgraph-io/dgraph/query 159.218s
We are using
json.Marshal()
in our code. This function is very generic function and can be used tomarshal any type. For our use case, most of the time, this is called for
string
type. When we dig deeper into the function's code, most of the time is spent on finding the right encoder function for type. In our case stringEncoder.This PR takes away the code of
encodeState.string()
and defines internally, which is just called forstring
type. Here are the benchmarks:This change is