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

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 :-)

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

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.

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.

@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.

Probably worth adding an initialize_copy

Good catch.