Skip to content

Comments

Avoid calling to_s at end of ERB#45756

Merged
jhawthorn merged 1 commit intorails:mainfrom
jhawthorn:avoid_to_s_in_erb
Aug 4, 2022
Merged

Avoid calling to_s at end of ERB#45756
jhawthorn merged 1 commit intorails:mainfrom
jhawthorn:avoid_to_s_in_erb

Conversation

@jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Aug 3, 2022 •
edited
Loading

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
@jhawthorn
Avoid calling to_s at end of ERB
ee68644 
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 casperisfine mentioned this pull request Aug 4, 2022
Merged
@casperisfine
Copy link
Contributor

casperisfine commented Aug 4, 2022

🤦 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

casperisfine commented Aug 4, 2022

But I'm good with merging this as is 👍

@jhawthorn
Copy link
Member Author

jhawthorn commented Aug 4, 2022

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 kaspth mentioned this pull request Oct 9, 2023
Closed
kaspth added a commit to bullet-train-co/jbuilder-schema that referenced this pull request Oct 9, 2023
@kaspth
Fix Rails 7.1 compatibility.
08804ee 
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 kaspth mentioned this pull request Oct 9, 2023
Merged
kaspth added a commit to bullet-train-co/jbuilder-schema that referenced this pull request Oct 9, 2023
@kaspth
Fix Rails 7.1 compatibility. (#57)
0fead8c 
* 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
@amatsuda amatsuda mentioned this pull request Nov 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@byroot byroot byroot approved these changes

Assignees

No one assigned

Labels

actionview

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jhawthorn @casperisfine @byroot