-
-
Notifications
You must be signed in to change notification settings - Fork 250
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
Parser microoptimizations #2246
Conversation
@@ -348,7 +348,7 @@ object Parsers extends SelectionParsers { | |||
schemaExtension | typeExtension | |||
|
|||
def definition(implicit ev: P[Any]): P[Definition] = | |||
executableDefinition | typeSystemDefinition | typeSystemExtension | |||
typeSystemDefinition | typeSystemExtension |
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.
It doesn't match the spec definition
(keeping the code close to the spec is good for maintainability), how about changing document
instead?
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.
I reverted the change. tbh it only makes a difference for invalid queries really
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.
But how about changing document
to (Start ~ definition.rep ~ End)
? I agree with the fix, just wanted to do it somewhere else 😄
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.
document
is currently this:
((Start ~ executableDefinition.rep ~ End) | (Start ~ definition.rep ~ End)).map(seq => ParsedDocument(seq.toList))
The reason for having Start ~ executableDefinition.rep ~ End
separately is so that when when we reach the end of the file after executableDefinition
has successfully parsed (which is the most common thing we parse), we shortcut to the end and don't try typeSystemDefinition
prior to exiting. I'm not sure if it's even possible, but in the case that there is a typeSystem definition in the query then we'll fallback to the full parser.
I actually realised that the "fix" I did earlier wouldn't work for cases that there is a mix of them; although I don't know if that's a valid query 🤔. If yes I should add a test for it
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.
Ah, didn't know we were trying to be clever 🤣
I was implementing a parser using fastparse today at $WORK and found out that
CharsWhileIn("xx", minChars)
is much more efficient than using.repX
. With these changes we get ~5-10% performance increase in our ParserBenchmark.Note that I also changed the
fullIntrospectionQuery
val to strip the margins of the query. I realised that we were benchmarking too heavily the whitespace parser