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 calling to_s at end of ERB #45756

Merged
merged 1 commit into from Aug 4, 2022
Merged

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Aug 3, 2022

With the recent changes to OutputBuffer, calling to_s is extremely expensive if the buffer later is concatenated to (if the buffer never changes it should be relatively inexpensive, as Ruby will share memory).

This tries to avoid calling to_s where possible, and to explicitly return nil when a buffer is passed to template.render

# frozen_string_literal: true

require 'rails'
require 'action_view'
require 'benchmark/ips'

class BenchView < ActionView::Base
  def compiled_method_container
    self.class
  end

  def self.compiled_method_container
    self
  end
end

view = BenchView.new(
  ActionView::LookupContext,
  {},
  nil,
)
locals = { message: "There aren’t any open alerts.!" }
erb_template = <<~ERB
    Hello! #{"a"*1000}
ERB
template = ActionView::Template.new(
  erb_template,
  "hello template",
  ActionView::Template::Handlers::ERB.new,
  virtual_path: "hello",
  locals: locals.keys,
)

def render_many(view, locals, template)
  buffer = ActionView::OutputBuffer.new
  1000.times do
    template.render(view, locals, buffer)
  end
end

Benchmark.ips do |x|
  x.report("render_many") { render_many(view, locals, template) }
end

Before

$ be ruby benchmark_output_buffer.rb
Warming up --------------------------------------
         render_many     1.000  i/100ms
Calculating -------------------------------------
         render_many     13.724  (± 7.3%) i/s -     69.000  in   5.071279s

After

$ be ruby benchmark_output_buffer.rb
Warming up --------------------------------------
         render_many    89.000  i/100ms
Calculating -------------------------------------
         render_many    929.182  (± 2.4%) i/s -      4.717k in   5.080056s

@rails-bot rails-bot bot added the actionview label Aug 3, 2022
With the recent changes to OutputBuffer, calling `to_s` is extremely
expensive if the buffer later is concatenated to (if the buffer never
changes it should be relatively inexpensive, as Ruby will share memory).
@jhawthorn jhawthorn marked this pull request as ready for review August 3, 2022 23:24
@jhawthorn jhawthorn requested a review from byroot August 3, 2022 23:50
@casperisfine
Copy link
Contributor

🤦 I definitely didn't think of shared string while writing this.

I'm definitely good with returning the buffer itself, rendering partials directly into the parent buffer is one of the things I'm trying to achieve with these refactors, so 👍 .

Until we get there though, we might need to make sure OutputBuffer can be concatenated into another buffer without having to dup the internal string.

@casperisfine
Copy link
Contributor

But I'm good with merging this as is 👍

@jhawthorn
Copy link
Member Author

Until we get there though, we might need to make sure OutputBuffer can be concatenated into another buffer without having to dup the internal string.

I think that should be fine as template.render will still call to_s if a parent buffer isn't provided

@jhawthorn jhawthorn merged commit 668d866 into rails:main Aug 4, 2022
kaspth added a commit to bullet-train-co/jbuilder-schema that referenced this pull request Oct 9, 2023
Rails 7.1 has sped up its internal OutputBuffer implementation (what handles << concatenation in templates),
which we rely on tricking a bit. rails/rails#45614

Then a follow up PR the `to_s` call was moved to after calling `view._run` rails/rails#45756

However, we rely on the return value being a Hash (which I'm not sure how to fix yet).

So since `target!` is the return value of our templates, we can wrap that and ensure `to_s` doesn't actually `to_s`, which Rails 7.1
will call for us.

On Rails < 7.1 we must call the compatibility `unwrap_target!` method ourselves to get our inner Hash return value.

This also adds a Rails version matrix so we can detect these things before release in the future.
kaspth added a commit to bullet-train-co/jbuilder-schema that referenced this pull request Oct 9, 2023
* Fix Rails 7.1 compatibility.

Rails 7.1 has sped up its internal OutputBuffer implementation (what handles << concatenation in templates),
which we rely on tricking a bit. rails/rails#45614

Then a follow up PR the `to_s` call was moved to after calling `view._run` rails/rails#45756

However, we rely on the return value being a Hash (which I'm not sure how to fix yet).

So since `target!` is the return value of our templates, we can wrap that and ensure `to_s` doesn't actually `to_s`, which Rails 7.1
will call for us.

On Rails < 7.1 we must call the compatibility `unwrap_target!` method ourselves to get our inner Hash return value.

This also adds a Rails version matrix so we can detect these things before release in the future.

* Allow us to run tests in multiple Rails versions

* Yep, I'm calling it, don't think it's worth it anymore

* Fix enum invocation for Rails 6.1 tests
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

3 participants