Optimize Indexable#join when all elements are strings

[asterite]

Fixes #6634

This PR overrides Indexable(T)#join and if:

• T is a String or
• T is a union that contains String, and all elements are strings
  then an optimized version of join is executed. In this case we can compute the total bytesize of the returned string: separator.bytesize * (array.size - 1) + array.sum(&.bytesize). This prevents potential string reallocations that happen in the default algorithm when String.build is used.

I used a slightly modified version of the code in #6634 to benchmark this:

require "benchmark"

cols = 10
rows = 1000
data = Array.new(rows) { Array.new(cols) { ("x"*1000) } }

Benchmark.ips do |x|
  x.report("doit") do
    csv = data.map { |row| row.join(",") }.join("\n")
  end
end

Before:

doit  91.19  ( 10.97ms) (± 1.70%)  75882435 B/op  fastest

After:

doit 183.46  (  5.45ms) (± 9.41%)  20050063 B/op  fastest

(note the allocated memory per op too)

In Ruby (change require "benchmark" to `require "benchmark/ips" using this gem):

                doit    117.854  (±10.2%) i/s -    585.000  in   5.030671s

So now it's faster than Ruby. Just note that if you run the original code from #6634 Crystal seems to be just a little slower than Ruby depending on the run, but a proper benchmark (Benchmark.ips) shows the real stuff.

Further optimization

There are two cases when this optimization is applied:

• T is a String or
• T is a union that contains String, and all elements are strings

The first one will be trivially faster than the previous one.

The second one involves an all?(&.is_a?(String)) check. I still think doing this check is faster than reallocating memory, so that's probably good. Ruby always does this check (it has no compile-time information) but while doing it it also computes the expected total bytesize, and switches to the default/slow implementation if an element is not a string. We could do the same, computing the length while checking .is_a?(String) but I think that will make the code a lot harder to read and understand, so maybe it's not worth it. But we could eventually do it if we find it's a significant performance difference (I doubt it).

Also, when there's no String in the array, the implementation is the default (slow) one.

[asterite]

I wanted to use super here but #6636

[Sija]

typo: btyesize

[asterite]

Thanks, fixed!

[RX14]

This is wrong because the size of the seperator isn't added. The specs pass though, is there any way to test this in the specs?

[asterite]

Oh, good catch! Yes, I'll check it now.

[asterite]

Fixed!

[asterite]

I need this in Indexable#join so I made it :nodoc:. It might be confusing to expose it as a public method because calling size always gets you the correct size. We can discuss exposing this in a separate issue/PR.

[asterite]

OK, this PR got a tad bigger now. @RX14 found that I had a bug when computing a string's size. So:

• I first fixed that but first checking that the sizes (and just in case bytesizes) match
• I extracted a private method to do it
• then I remembered we also check for size and bytesize equality for stirngs in string_spec.cr, because we manually construct strings in String
• I thought about having an eq_string matcher to compare strings, adding it to the support directory and requiring it when needed, but that's bug prone: what if you compare two strings and forget to check for their bytesize and size, and you also forget to require that file from support?
• so I then changed the eq matcher to always perform a bytesize and size equality when comparing strings

This actually uncovered a bug in String#gsub(Char, Char). So I think having eq do this comparison by default is good because we create such strings in the whole standard library, but anyone else is also free to use String.new in their shards and they might put incorrect values for bytesize and size there. The eq matcher got a bit more complex, and it might be just slightly slightly slower, but I'm pretty sure it's insignificant.

[RX14]

I don't like the convention of using _name for private names when crystal already has private methods, just private def join_strings is fine.

[asterite]

The problem with private methods is that they can still be accessed by subclasses or including types. But yeah, I'll change it.