-
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
fix(Chunker): JSON parsing performance #7171
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 4 of 4 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karlmcguire and @vvbalaji-dgraph)
go.mod, line 19 at r2 (raw file):
github.com/blevesearch/bleve v1.0.13 github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd github.com/davecgh/go-spew v1.1.1
remove?
go.mod, line 48 at r2 (raw file):
github.com/hashicorp/vault/api v1.0.4 github.com/minio/minio-go/v6 v6.0.55 github.com/minio/simdjson-go v0.1.5
remove?
go.mod, line 52 at r3 (raw file):
github.com/pkg/profile v1.2.1 github.com/prometheus/client_golang v0.9.3 github.com/prometheus/common v0.4.1 // indirect
revert go.mod. And then run go mod tidy.
chunker/json_parser.go, line 385 at r3 (raw file):
for k, v := range m { if strings.Contains(k, x.FacetDelimeter) { mf[k] = v
I'd also delete them from m, so the iteration over m is faster.
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karlmcguire and @vvbalaji-dgraph)
chunker/json_parser.go, line 433 at r3 (raw file):
// option. // We also skip facets here because we parse them with the corresponding predicate. if pred == "uid" || strings.Index(pred, x.FacetDelimeter) > 0 {
If m doesn't have facets, then you can save on the strings.Index search.
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.
Reviewable status: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @vvbalaji-dgraph)
go.mod, line 19 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
remove?
Done.
go.mod, line 48 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
remove?
Done.
go.mod, line 52 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
revert go.mod. And then run go mod tidy.
Done.
chunker/json_parser.go, line 385 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
I'd also delete them from m, so the iteration over m is faster.
Done.
chunker/json_parser.go, line 433 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
If m doesn't have facets, then you can save on the strings.Index search.
Done.
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.
Nice! Feel free to squash and merge.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
* testing simdjson speed, saw a 4x improvement from encoding/json speed * first pass finding facets * walking the tree, currently not passing all tests * walk function passes all tests * three dimensional walk map * 87% speed increase now * uncomment testing function * cleanup and documentation * formatting fixes * ignore deepsource warning * fix tests * simpler fix * tidy * remove comment * delete from main map for faster iteration * fix go.mod
Fixes DGRAPH-2810.
About a 90% performance increase in NQuads/sec on the data used in the
BenchmarkNoFacets()
function.This change is