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

Avoid checking defined?(@html_safe) on Ruby 3+ #45620

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

jhawthorn
Copy link
Member

We should not need to check defined? here because we are only interested in whether @html_safe is truthy or falsy.

In Ruby versions >= 3.0, there is no warning on access of undefined ivars. On previous versions, it can emit a warning.

cc @casperisfine @noahgibbs

$ cat benchmark_html_safe.rb
require 'rails'
require 'action_view'
require 'benchmark/ips'

SAFE_BUFFER = ActiveSupport::SafeBuffer.new

Benchmark.ips do |x|
  x.report("SAFE_BUFFER.html_safe?", "SAFE_BUFFER.html_safe?")
end

Before

$ be ruby benchmark_html_safe.rb
Warming up --------------------------------------
SAFE_BUFFER.html_safe?
                         1.300M i/100ms
Calculating -------------------------------------
SAFE_BUFFER.html_safe?
                         12.884M (± 0.8%) i/s -     65.005M in   5.045850s

After

$ be ruby benchmark_html_safe.rb
Warming up --------------------------------------
SAFE_BUFFER.html_safe?
                         2.456M i/100ms
Calculating -------------------------------------
SAFE_BUFFER.html_safe?
                         24.336M (± 1.0%) i/s -    122.821M in   5.047382s

@noahgibbs
Copy link
Contributor

LGTM either way.

@jhawthorn
Copy link
Member Author

jhawthorn commented Jul 18, 2022

With Jean's attr_reader suggestion.

$ be ruby benchmark_html_safe.rb
Warming up --------------------------------------
SAFE_BUFFER.html_safe?
                         2.989M i/100ms
Calculating -------------------------------------
SAFE_BUFFER.html_safe?
                         29.188M (± 1.3%) i/s -    146.468M in   5.018946s

end
attr_reader :html_safe
alias_method :html_safe?, :html_safe
undef_method :html_safe
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually realizing we need remove_method, not undef_method, because we want this to fall through to ::String's implementation of html_safe 😅

Copy link
Member

Choose a reason for hiding this comment

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

Right, I always have to check the doc when I need either of these. I can never remember which is which.

We should not need to check defined? here because we are only interested
in whether @html_safe is truthy or falsy.

We can  use an aliased attr_reader to make this even faster by skipping
both method dispatch.

Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
@jhawthorn jhawthorn merged commit b5a758d into rails:main Jul 19, 2022
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.

None yet

4 participants