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

[Language::Visitor] use method-based visit handlers #1290

Merged
merged 14 commits into from
Jul 10, 2018

Conversation

rmosolgo
Copy link
Owner

I'm thinking of stealing a page from the parser gem's book. There, the AST visitor is a class with on_* handlers for each kind of AST node. It's really easy to get going, I'd like to do the same thing in GraphQL-Ruby.

So, I tried the API on a couple of validation rules, and I'm going to try a few more. If it seems to work, I'll improve the support for that kind of API and deprecate the [node_class] << API. I think we will need to use yield or super to support [node_class].leave <<. (parser uses super). Also, I'll be interested to see the performance impact.

@arthurschreiber
Copy link
Contributor

This is rad! ❤️I really love this change.

@olleolleolle
Copy link
Contributor

This is a helpful change.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Apr 2, 2018

I rewrote StaticValidation to use class-based visitor, I'm happy with the code and the perf is about the same

~/code/graphql-ruby $ git pom
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
From github.com:rmosolgo/graphql-ruby
 * branch              master     -> FETCH_HEAD
Already up-to-date.
~/code/graphql-ruby $ be rake bench:validate && be rake bench:validate
Warming up --------------------------------------
validate - introspection
                        27.000  i/100ms
validate - abstract fragments
                        63.000  i/100ms
validate - abstract fragments 2
                        38.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        279.931  (± 7.9%) i/s -      1.404k in   5.051684s
validate - abstract fragments
                        652.288  (± 5.1%) i/s -      3.276k in   5.035280s
validate - abstract fragments 2
                        393.109  (± 8.1%) i/s -      1.976k in   5.065726s
validate - big query     37.769  (± 5.3%) i/s -    189.000  in   5.017028s
Warming up --------------------------------------
validate - introspection
                        27.000  i/100ms
validate - abstract fragments
                        62.000  i/100ms
validate - abstract fragments 2
                        39.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        278.284  (± 4.7%) i/s -      1.404k in   5.055952s
validate - abstract fragments
                        635.385  (± 5.4%) i/s -      3.224k in   5.088037s
validate - abstract fragments 2
                        388.317  (± 5.2%) i/s -      1.950k in   5.035392s
validate - big query     37.307  (± 5.4%) i/s -    189.000  in   5.079340s
~/code/graphql-ruby $ git co class-based-visitor
Switched to branch 'class-based-visitor'
~/code/graphql-ruby $ be rake bench:validate && be rake bench:validate
Warming up --------------------------------------
validate - introspection
                        28.000  i/100ms
validate - abstract fragments
                        66.000  i/100ms
validate - abstract fragments 2
                        40.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        288.311  (± 4.5%) i/s -      1.456k in   5.060300s
validate - abstract fragments
                        677.300  (± 4.9%) i/s -      3.432k in   5.078672s
validate - abstract fragments 2
                        404.773  (± 4.9%) i/s -      2.040k in   5.052892s
validate - big query     37.714  (± 2.7%) i/s -    189.000  in   5.017786s
Warming up --------------------------------------
validate - introspection
                        29.000  i/100ms
validate - abstract fragments
                        68.000  i/100ms
validate - abstract fragments 2
                        42.000  i/100ms
validate - big query     3.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        291.989  (± 4.1%) i/s -      1.479k in   5.074170s
validate - abstract fragments
                        684.999  (± 3.9%) i/s -      3.468k in   5.070835s
validate - abstract fragments 2
                        412.762  (± 4.8%) i/s -      2.100k in   5.098861s
validate - big query     37.823  (± 5.3%) i/s -    189.000  in   5.013675s
~/code/graphql-ruby $

@rmosolgo rmosolgo changed the base branch from master to 1.8-dev April 2, 2018 20:39
@rmosolgo
Copy link
Owner Author

rmosolgo commented Apr 2, 2018

I pushed a little harder and re-wrote the remaining bits to use the new API, now it's a decent bit faster, 10% give or take.

~/code/graphql-ruby $ be rake bench:validate && be rake bench:validate
Warming up --------------------------------------
validate - introspection
                        32.000  i/100ms
validate - abstract fragments
                        76.000  i/100ms
validate - abstract fragments 2
                        46.000  i/100ms
validate - big query     4.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        322.026  (± 3.7%) i/s -      1.632k in   5.075263s
validate - abstract fragments
                        744.823  (± 5.0%) i/s -      3.724k in   5.013195s
validate - abstract fragments 2
                        452.471  (± 6.0%) i/s -      2.254k in   5.000423s
validate - big query     40.256  (± 5.0%) i/s -    204.000  in   5.084613s
Warming up --------------------------------------
validate - introspection
                        32.000  i/100ms
validate - abstract fragments
                        76.000  i/100ms
validate - abstract fragments 2
                        45.000  i/100ms
validate - big query     4.000  i/100ms
Calculating -------------------------------------
validate - introspection
                        320.540  (± 4.4%) i/s -      1.600k in   5.001771s
validate - abstract fragments
                        756.113  (± 4.0%) i/s -      3.800k in   5.033471s
validate - abstract fragments 2
                        454.004  (± 5.3%) i/s -      2.295k in   5.069387s
validate - big query     39.463  (± 5.1%) i/s -    200.000  in   5.082648s

@rmosolgo
Copy link
Owner Author

rmosolgo commented Apr 3, 2018

I removed TypeStack, but I should put it back, graphql-client depends on it.

@rmosolgo rmosolgo changed the title [Language::Visitor] experiement with method-based visit handlers [Language::Visitor] experiment with method-based visit handlers Apr 5, 2018
@rmosolgo rmosolgo changed the base branch from 1.8-dev to master April 5, 2018 13:46
@rmosolgo rmosolgo added this to the 1.9.0 milestone Jul 9, 2018
@rmosolgo rmosolgo changed the base branch from master to 1.9-dev July 9, 2018 19:16
@rmosolgo rmosolgo mentioned this pull request Jul 9, 2018
7 tasks
@rmosolgo rmosolgo changed the title [Language::Visitor] experiment with method-based visit handlers [Language::Visitor] use method-based visit handlers Jul 10, 2018
@rmosolgo
Copy link
Owner Author

This is almost passing. We still need one more thing out of the new visitor API, a way to modify the AST that's being iterated over. But I will add that in a later PR, this is a big step forward as-is!

@rmosolgo rmosolgo merged commit 82cbc7f into 1.9-dev Jul 10, 2018
@rmosolgo rmosolgo deleted the class-based-visitor branch July 10, 2018 19:43
rmosolgo pushed a commit that referenced this pull request Sep 13, 2018
[Language::Visitor] use method-based visit handlers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants