Speedup ActionView::OutputBuffer

[casperisfine]

MRI has a lot of optimizations for string concatenation that are only available when concatenating into a String instance.

Using a String subclass disable these optimizations.

The difference is even more important in Ruby 3.2 where both YJIT and the VM noticeably improve String#<< performance.

So ideally we want the buffer not to be a String subclass. Luckily, the Action View buffer is for internal use only, so we can replace inheritance by composition without much work.

Benchmark:

ActionView::OutputBuffer:   147644.2 i/s
    optimized buffer:   228001.4 i/s - 1.54x  (± 0.00) faster

Source: https://gist.github.com/casperisfine/7199579a138e268fda71d6a91366af49

NB: That 50% faster figure is to be contextualized, it can radically change
from one template to the other, but is always faster.

cc @jhawthorn @matthewd @noahgibbs @tenderlove

[jhawthorn]

I don't think this change is quite right. Previously this was actually mutating the output buffer (it's a really strange thing to do). @seejohnrun and I ran into this as well when experimenting with modifying outputbuffer, and tried replacing what this method does capture https://github.com/jhawthorn/rails/pull/1/files#diff-7e8f6bc4873643df271aa007b513b993a8e424964b76e4eec2c1d156d9108ac3R252 (but I don't remember if it works)

[casperisfine]

I'll look more into it tomorrow, but I was under the impression that mutating the buffer was simply an unneeded side effect. I'm pretty sure that buffer instance is no longer used after that slice!.

[casperisfine]

Ok so I was wrong. This slice! is to handle formats such as .builder, as it doesn't access @output_buffer, it is initialized once with it.

So in cache do we can't just substitute the buffer, we have to mutate it.

That's really unfortunate.

I'm tempted to add some stack to the buffer, the problem is that it's not clear to be wether @output_buffer is guaranteed to be an OutputBuffer (or StreamBuffer) as there are some tests using ActiveSupport::SafeBuffer. I'd have to dig into that as well to see wether it's legit or not.

[jhawthorn]

I like this change. I think the speedup is more avoiding the indirection than the concatenation itself (I don't think anything called by rb_str_concat checks the class, just the builtin type), but equally worthwhile! 🚀

I don't know if this happens in practice but here's another behaviour change we should guard against:

b1 = ActionView::OutputBuffer.new
b1 << "foo"
b2 = b1.dup
b1 << "bar"

p b1.to_s # => "foobar"
p b2.to_s # => "foobar"

Probably worth adding an initialize_copy definition to ActionView::OutputBuffer

[noahgibbs]

John: one of many reasons we're hoping to avoid concatenating onto a non-basic string is that YJIT is significantly faster when the receiver is a string. That's not the only reason we're doing it, but it's a side benefit :-)

[jhawthorn]

John: one of many reasons we're hoping to avoid concatenating onto a non-basic string is that YJIT is significantly faster when the receiver is a string. That's not the only reason we're doing it, but it's a side benefit :-)

As I said, I'm in favour of this change. But I don't think that's true. String concatenation onto a subclass can be more or less the same speed.

EDIT:

# frozen_string_literal: true
require 'benchmark/ips'

class SafeBuffer < String
end

class OutputBuffer < SafeBuffer
  def <<(value)
    return self if value.nil?
    super(value.to_s)
  end
end

str = +""
safe_buffer = SafeBuffer.new
output_buffer = OutputBuffer.new
safe_buffer_encoded = SafeBuffer.new("")
output_buffer_encoded = OutputBuffer.new("")

puts "Encodings:"
pp({str:, safe_buffer:, output_buffer:, safe_buffer_encoded:, output_buffer_encoded:}.transform_values(&:encoding))

Benchmark.ips do |x|
  x.report("String") { str << "" }
  x.report("SafeBuffer") { safe_buffer << "" }
  x.report("OutputBuffer") { output_buffer << "" }
  x.report("SafeBuffer (same encoding)") { safe_buffer_encoded << "" }
  x.report("OutputBuffer (same encoding)") { output_buffer_encoded << "" }
  x.compare!(order: :baseline)
end

$ ./ruby --yjit benchmark_string_concat.rb
Encodings:
{:str=>#<Encoding:UTF-8>,
 :safe_buffer=>#<Encoding:ASCII-8BIT>,
 :output_buffer=>#<Encoding:ASCII-8BIT>,
 :safe_buffer_encoded=>#<Encoding:UTF-8>,
 :output_buffer_encoded=>#<Encoding:UTF-8>}
Warming up --------------------------------------
              String   573.166k i/100ms
          SafeBuffer   366.353k i/100ms
        OutputBuffer   360.232k i/100ms
SafeBuffer (same encoding)
                       597.930k i/100ms
OutputBuffer (same encoding)
                       582.716k i/100ms
Calculating -------------------------------------
              String      5.989M (± 1.1%) i/s -     30.378M in   5.073130s
          SafeBuffer      3.787M (± 1.2%) i/s -     19.050M in   5.031701s
        OutputBuffer      3.712M (± 0.9%) i/s -     18.732M in   5.046107s
SafeBuffer (same encoding)
                          5.985M (± 1.5%) i/s -     30.494M in   5.096533s
OutputBuffer (same encoding)
                          5.915M (± 1.2%) i/s -     29.719M in   5.024993s

Comparison:
              String:  5988771.9 i/s
SafeBuffer (same encoding):  5984749.7 i/s - same-ish: difference falls within error
OutputBuffer (same encoding):  5915046.7 i/s - same-ish: difference falls within error
          SafeBuffer:  3786659.5 i/s - 1.58x  (± 0.00) slower
        OutputBuffer:  3712492.5 i/s - 1.61x  (± 0.00) slower

[noahgibbs]

Okay. I don't think I have permissions on that repo. But at a minimum we would need to build more infrastructure for YJIT to make that true. So with current YJIT as it exists, I'm pretty certain that's true.

[jhawthorn]

benchmark attached, but again, I am in favour of this change.

I also don't think YJIT needs extra infrastructure for that to be true, because Ruby does not special case rb_cString for concatenation (just T_STRING). Even if it did (it doesn't) we are going to want it because ActiveSupport::SafeBuffer is public and a string subclass.

Let's continue the YJIT part of this discussion elsewhere because I think this is a good change anyways.

[casperisfine]

@jhawthorn this is the optimization I'm talking about: https://github.com/ruby/ruby/blob/85ea46730deff70172a9f50172f0011a7401f371/vm_insnhelper.c#L5374-L5380

static VALUE
vm_opt_ltlt(VALUE recv, VALUE obj)
{
    if (SPECIAL_CONST_P(recv)) {
	return Qundef;
    }
    else if (RBASIC_CLASS(recv) == rb_cString &&
	     BASIC_OP_UNREDEFINED_P(BOP_LTLT, STRING_REDEFINED_OP_FLAG)) {
	if (LIKELY(RB_TYPE_P(obj, T_STRING))) {
	    return rb_str_buf_append(recv, obj);
	} else {
	    return rb_str_concat(recv, obj);
	}
    }
    else if (RBASIC_CLASS(recv) == rb_cArray &&
	     BASIC_OP_UNREDEFINED_P(BOP_LTLT, ARRAY_REDEFINED_OP_FLAG)) {
	return rb_ary_push(recv, obj);
    }
    else {
	return Qundef;
    }
}

String subclasses are explictly excluded, and it's particularly true for SafeBuffer as << is redefined.

[casperisfine]

Probably worth adding an initialize_copy

Good catch.

[jhawthorn]

We ran into a pretty significant performance regression with this change.

In our app, we usually are able to call template.render with the buffer argument in order to re-use the same buffer of the parent template. This is desirable because it allows deeply nesting templates without paying any cost for re-copying large buffers between strings.

However, with this change OutputBuffer#to_s is no longer a no-op, and allocates a new string. Also, we configure Erubi to call to_s on the output buffer at the end of each template. When using a new output buffer for each template, this works great as we're able to share the internal String memory between the output_buffer internal buffer and the newly output SafeBuffer. However, if that buffer is continued to be used it will require fully copying the memory to a new string, causing significant GC pressure and spending a lot of time in memcpy.

Here's a benchmark demonstrating the issue:

# 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 = { }
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 this change

$ be ruby benchmark_output_buffer.rb
Warming up --------------------------------------
         render_many    90.000  i/100ms
Calculating -------------------------------------
         render_many    907.139  (± 2.4%) i/s -      4.590k in   5.063373s

After this change

$ be ruby benchmark_output_buffer.rb
Warming up --------------------------------------
         render_many     1.000  i/100ms
Calculating -------------------------------------
         render_many     14.304  (± 7.0%) i/s -     72.000  in   5.068334s

---

We can't run this change in production as-is. The good news is that I think we might be able to fail-forward by avoiding calling to_s at the end of the ERB template. (I'll try to open a PR shortly)

[casperisfine]

Thanks @jhawthorn I'll comment on #45756