Skip to content
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

Improve query parsing performance #351

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Improve query parsing performance #351

merged 1 commit into from
Mar 26, 2018

Conversation

guymers
Copy link
Contributor

@guymers guymers commented Mar 26, 2018

Use the fact that lineIdx is a sorted array to more efficiently calculate the previous cursor and the number of cursors between it and 0. This gives an approximate 3.5x speedup when parsing the kitchen sink query.

@OlegIlyenko
Copy link
Member

OlegIlyenko commented Mar 26, 2018

Thanks for the working on this improvement! In general, the change looks good to me. 👍

From the very beginning, I strived to keep the project build as simple as possible. This has a number of advantages and makes it easier for newcomers to learn and contribute to the project. So I have a concern about the introduction of the multi-module build.

I would suggest to keep the PR simpler and focus only on the actual code changes. At the moment I would prefer to have a benchmark as a separate project. If the benchmark would become bigger and there would be a strong argument in favor of its inclusion in than the main build, I think we can reconsider it and discuss it as a separate issue.

What do you think about it? Would it be ok for you to just keep changes in the PositionTracking.scala?

Use the fact that `lineIdx` is a sorted array to more efficiently
calculate the previous cursor and the number of cursors between it and
0. This gives an approximate 3.5x speedup when parsing the kitchen sink
query.
@guymers
Copy link
Contributor Author

guymers commented Mar 26, 2018

Sure, if you don't want to a have a multi-module build. I added the benchmark module so the performance of the change could be verified. It is a separate commit so I can easily remove it.

@OlegIlyenko
Copy link
Member

Thanks again!

@OlegIlyenko OlegIlyenko merged commit 048dc64 into sangria-graphql:master Mar 26, 2018
@OlegIlyenko OlegIlyenko added this to the v1.4.1 milestone Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants