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

Eliminate dynamic dispatch from Language::Visitor and Analysis::AST::Visitor #4338

Merged
merged 3 commits into from
Feb 12, 2023

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Feb 8, 2023

In analysis, Kernel#public_send was a big delay (and it was own time -- overhead -- not the methods that end up getting called). It could mostly be eliminated by some heavy-handed codegen. It seems to me that it pays off:

Before (3.6% own time)

stackprof before-stackprof.dump --method=Kernel#public_send
Kernel#public_send (<cfunc>:1)
  samples:  3518 self (3.3%)  /   52917 total (49.9%)
  callers:
    232534  (  439.4%)  GraphQL::Language::Visitor#visit_node
    22700  (   42.9%)  GraphQL::Analysis::AST::Visitor#call_analyzers
    4106  (    7.8%)  GraphQL::Schema::Warden.visible_entry?

After (0.5% own time)

stackprof last-stackprof.dump --method=Kernel#public_send
Kernel#public_send (<cfunc>:1)
  samples:   507 self (0.5%)  /   44524 total (47.5%)
  callers:
    44524  (  100.0%)  GraphQL::Language::Visitor#visit
    3827  (    8.6%)  GraphQL::Schema::Warden.visible_entry?

TODO:

  • Clean up the new Language::Visitor code. I think those visit_{...}_children methods could be dynamically created unless the method was already manually defined, and that would remove the need for the custom method name argument. Also reconsider the method body organization to make sure stuff that can be de-duplicated is.
  • Generate methods for Analysis::AST::Visitor to reduce duplication?
  • Consider an OO approach to this, for example, an API like node.visit # visit this node and all children recursively. It may be just a matter of ergonomics 🤷

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.

1 participant