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

PoC: Optimize predicate methods with attr_reader #5082

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amomchilov
Copy link
Contributor

PoC for #5081

Comment on lines +20 to +22
attr_reader :abstract
alias_method :abstract?, :abstract
remove_method :abstract
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also possible to make a helper to automate these, but then IDEs won't know about the presence of the generated methods.

module PredicateHelper # This can be mixed into Module, if you're into monkey-patching
  # Given `"foo?"`, this method generates an optimized method that reads from `@foo`.
  def attr_predicate(predicate_name)
    raise ArgumentError unless predicate_name.end_with?("?")
    
    attr_method_name = predicate_name.to_s.delete_suffix("?")
    attr_reader(attr_method_name)
    alias_method(predicate_name, attr_method_name)
    remove_method(attr_method_name)
  end
end

@rmosolgo
Copy link
Owner

rmosolgo commented Sep 2, 2024

Hey, thanks for proposing this change! I'm definitely interested in it. Another implementation might be write a Rubocop rule which identifies and updates code that uses the "bad" approach. There are a couple other development-related cops here in case you're interested in try that: https://github.com/rmosolgo/graphql-ruby/tree/master/cop/development

@amomchilov
Copy link
Contributor Author

amomchilov commented Sep 3, 2024

@rmosolgo Happy to hear! This is definitely rubocop-able. I'd even go so far as to add it upstream to https://github.com/rubocop/rubocop-performance

Would you mind running a quick benchmark on this, to see if it's worthwhile? Btw I wrote up more details in the issue: #5081

I have some env issues, and I won't have time to sort them out until at least next week

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.

2 participants