Improve lexer performance by using a byte array as source #137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @Matthias247 Thanks for taking your time to dive deep into this and submitting a PR, appreciate it very much 👍 Briefly glancing through the changes, it looks good. I'll take the time to review it ASAP. Cheers! |
This was referenced Jun 11, 2016
Open
sogko
added a commit
to sogko/graphql
that referenced
this pull request
Jun 12, 2016
…h commit graphql-go#137 - Increased coverage for `lexer` package to 100% - Added more tests to cover parsing unicode strings (graphql-go#135) as well. - Fixed invalid test for `lexer` properly escaping slashes for TokenString type
sogko
added a commit
that referenced
this pull request
Jun 12, 2016
Fix `lexer` tests (follow up to #137)
sogko
added a commit
to sogko/graphql
that referenced
this pull request
Jan 27, 2017
* master: gofmt -s executor: add graphql tag Fix ObjectConfig duplicate json tag. Name had a 'description' json tag which was duplicate Minor fix to stop `go vet` from complaining Add 1.7 and tip to build matrix for Go Minor fix to stop `go vet` from complaining README: add Documentation section Fixes tests that was broken with enhancements made to the `lexer` with commit graphql-go#137 Improve lexer performance by using a byte array as source Updated `graphql.Float` coercion - if value type is explicitly `float32`, return `float32` - if value type is explicitly `float64`, return `float64` - if value type is `string`, return `float64` - for everything else, use system-default
sogko
added a commit
to sogko/graphql
that referenced
this pull request
Jan 27, 2017
* sogko/0.6.0: (34 commits) gofmt -s executor: add graphql tag Fix ObjectConfig duplicate json tag. Name had a 'description' json tag which was duplicate Minor fix to stop `go vet` from complaining Add 1.7 and tip to build matrix for Go Minor fix to stop `go vet` from complaining README: add Documentation section Fixes tests that was broken with enhancements made to the `lexer` with commit graphql-go#137 Improve lexer performance by using a byte array as source Validation: improving overlapping fields quality (graphql-go#386) Validation: context.getFragmentSpreads now accepts selectionSet rather than fragment AST node Factor out more closure functions Factor out closure functions to normal functions Deprecated directive (graphql-go#384) Revert back directives to use `var`; `const` not applicable here Deprecated directive (graphql-go#384) RFC: Directive location: schema definition (graphql-go#382) RFC: Schema Language Directives (graphql-go#376) Update `NewSchema()` to use new exported introspective types Export introspection in public API ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I did some peformance testing for graphql-go and found out that it was superslow for my use-case (about 300ms for processing a simple query). After using the go profiling tools it was obvious that the Garbage Collector takes most of the time for it (> 250ms).
I have then viewed through the graphql related code and have seen that the Lexer produces an extreme amount of garbage. The reason is that the Lexer stores the request as a
stringtype but converts it to ´[]rune` every time it wants to read a char. This will cause an allocation and copy for reading each character!I have now made changes were the
Sourceis directly storing a[]bytearray, which can be indexed without performance penalty and which is never converted to a rune array. I changed some functions to read runes from the given offset and the source array and to be able to skip over them even if they are multibyte values.This pull request improves performance for me by factor 30! This means I'm coming to 10ms for processing a request, which is still not stellar but more workable. In the new performance traces the lexer is no longer the most time consuming part.
The code was not heavily reviewed, so I suggest someone else also takes a look at it before merging. But tests are working and my application with it is working too.