Implement string interpolation as a call to String.interpolation

[asterite]

Before this PR, a string interpolation like:

"hello #{world}!"

was roughly expanded to:

String.build do |io|
  io << "hello "
  io << world
  io << "!"
end

In this PR it's expanded to:

String.interpolation("hello ", world, "!")

The benefits of doing this are:

• We can change how string interpolation works without modifying the compiler: we just need to modify the definition of String.interpolation
• The way string interpolation works is more visible: anyone can go to the source code and see how it's done, in Crystal, instead of looking at the compiler's source code
• We can provide optimized overloads. Right now there are a few:
  • when interpolating a single string value, like "#{value}", we return the same value. This is uncommon, but why not?
  • similarly, interpolating a single non-string value just calls to_s on it: much more efficient than using String.build
  • when interpolating multiple strings we can precompute the total bytesize, something that the String.build approach can't do
  • when interpolating a char at the beginning or end we use + (uncommon, but why not?)
• Eliminates discussions like "Instead of doing "foo#{bar}" you should do "foo" + bar". String interpolation will provide the most performant implementation, always (and I plan to further optimize this by inlining constants if possible, but in a separate PR).

A small benchmark:

require "benchmark"

Benchmark.ips do |x|
  x.report("interpolation") do
    var1 = "hello"
    var2 = "world"
    "foo #{var1} bar #{var2} baz"
  end
end

Results:

before: interpolation   8.51M (117.49ns) (± 4.86%)  224B/op  fastest
after:  interpolation  21.81M ( 45.85ns) (± 3.71%)  48.0B/op  fastest

[bcardiff]

😻 now I need to update my next presentation that mention interpolation internals. just on time.

Any comparison whether it was better the inline approach for creating the string builder? Because after this PR there will many String.interpolation overloads.

[straight-shoota]

What's the purpose of this overload? It's equivalent to the non-restricted overload. String#to_s returns self.

[asterite]

You are right

[jkthorne]

I like the syntax of String["hello", world, "!"] to similar to other literals. But this could be a macro to call the interpolation method?

[straight-shoota]

Technically, making "#{value}" just return value is a breaking change, because previously it returned a new string. This is only relevant when doing unsafe stuff, but still might it be better to dup the string to avoid any inconsistencies?

[asterite]

@bcardiff Good question! I didn't measure how it affects compilation time or memory. On one hand the old approach generated more code to type (multiple calls). The new code just generates a call, which means that if in your program you have string interpolation with same types the compiler will have those instances cached. But then it will need to create a different tuple type for each combination. But compiling the compiler took the same time/memory, same for specs, so maybe it doesn't matter. I was going to mention all of this in this PR but I didn't because I didn't conclude anything about it.

[asterite]

Technically, making "#{value}" just return value is a breaking change, because previously it returned a new string.

Good point. But what's a case where you want to have a String with the same contents but different memory address?

[straight-shoota]

When you do unsafe stuff with it, such as passing it to a C library that modifies the string in place.

[asterite]

When you do unsafe stuff with it, such as passing it to a C library that modifies the string in place.

Let's try this change and see if it breaks something. Using "#{value}" should be pretty rare, more so doing that in a C interaction.

[asterite]

Or maybe we can just duplicate the string. I guess nobody would do "#{value}" if value is already a string... do doing that with a small performance penalty isn't bad.

[bew]

Also, what about moving this to another file string/interpolation.cr?

[vlazar]

Technically, making "#{value}" just return value is a breaking change, because previously it returned a new string. This is only relevant when doing unsafe stuff, but still might it be better to dup the string to avoid any inconsistencies?

While it's a breaking change, wouldn't it be better to still have this optimization in the long run?
The value.dup (which looks nicer) is the same as "#{value}", right?
I wonder how really common is the current "#{value}" usage?

[konovod]

Docs says that Strings are immutable (and that is good), so modifying them isn't just unsafe stuff (things that can break a contract but language "believes" a programmer that he don't), but undefined behavior (things that always break a contract). So there is no breaking change.

[vlazar]

Am I the only one who finds this implementation of String#dup (as well as clone) very confusing? Or is it just my Ruby background?

crystal/src/string.cr

Lines 4276 to 4278 in 0e2e1d0

	def dup
	self
	end

[asterite]

Strings are immutable so there's no point in dup allocating a new string. The more memory we can save, the better.

I also agree with reverting my last commit. I'm pretty sure it's harmless.

[wooster0]

This is great. I'm really looking forward to the constant inlining as well.

But should this maybe be called interpolate instead? String.interpolation reads nicely as "string interpolation", it's the string interpolation. But technically String.interpolate would be more appropriate I think because you call the method of String that interpolates. Similar to how you call the split method of a String instance so that it splits it. It would also be shorter.
And if you actually call the method it looks like this: String.interpolation("hello", "world") and in this case I really think String.interpolate("hello", world, "!") reads better (even though you are normally not supposed to call it) because you basically ask String to interpolate the following things for you, you don't ask it to "interpolation" the following things.

[asterite]

Yes, the name can be interpolate. I find String.interpolation to be slightly better because:

• it matches what it means: it's what a string interpolation is expanded to
• it doesn't interpolate anything. Interpolation is doing "foo #{bar}", that is, putting #{bar} inside of a string. So String.interpolate("foo ", bar) isn't exactly right because the interpolation is already done (by this exact expansion). So String.interpolation looks like it's the result of the interpolation.

[straight-shoota]

We could consider String.concat because that's what it does: it concatenates the arguments. But interpolation is fine as well.

[asterite]

concat was the first name I was going to use, so 👍 from me on that

[yxhuvud]

join might be a strong contender on the name front though.

EDIT: BTW, how does the general case compare to Tuple#join ? Would it make sense to do similar allocation size optimizations in Enumerable#join?

[straight-shoota]

@yxhuvud We can't optimize Enumerable because it doesn't necessarily return the same elements each time, thus would require to cache the elements and defeat the purpose. But Indexable#join is already optimized in a similar way as in this PR. It's just a bit more complex because of separators.

[asterite]

I think we could optimize Tuple#join and then make this PR use that, it's slightly more complex because I have to do the size and type checks all inside a single method. Plus I liked the idea of having a specific method representing the implementation of string interpolation.

[RX14]

Technically, making "#{value}" just return value is a breaking change

It's not a breaking change since you need to use unsafe methods to observe it.

I like the idea of this PR, but compile-time and other benchmarks would be great.

[straight-shoota]

Actually, Hash's new compare_by_identity could also observe this.

In case you were using "#{value}" to get a copy of a string which doesn't share its object id, this would no longer work (with String you can neither use #clone nor #dup to get a different instance).

[asterite]

I think nobody is doing "#{value}" where value is already a String with the use case of duplicating the string. I'm almost sure that if someone is doing that it's just to convert something to a String (because they maybe don't know about to_s?).

So I wouldn't worry about this. If it's a breaking change then it's fine, we just document what's the expected behavior and provide an upgrade path (Changelog: notice that "#{value}" doesn't create a new String instance if value is a String).

[straight-shoota]

Totally agree. It's fine to change it. But that's a breaking change.

[bcardiff]

I think nobody is doing "#{value}" where value is already a String

It might be a case of some polymorphic function that ends up doing that. Either way, we are fine with the new behaviour: returning the same object. Strings are immutable.

[bcardiff]

Although we still need #8400 (comment)

[straight-shoota]

... and I'd like to have at least some benchmarks on the performance impact on compile time and runtime.

[asterite]

I tried this code:

world = "hello"
baz = 1

{% for i in 1..100_000 %}
  "hello #{world} foo bar #{baz}"
{% end %}

Current compiler:

$ time crystal build foo.cr -s
Parse:                             00:00:00.000111332 (   0.25MB)
Semantic (top level):              00:00:00.326971158 ( 140.35MB)
Semantic (new):                    00:00:00.000738606 ( 140.35MB)
Semantic (type declarations):      00:00:00.041685849 ( 140.35MB)
Semantic (abstract def check):     00:00:00.003865196 ( 140.35MB)
Semantic (ivars initializers):     00:00:00.008136129 ( 140.35MB)
Semantic (cvars initializers):     00:00:00.052535028 ( 156.35MB)
Semantic (main):                   00:00:01.233419492 ( 476.78MB)
Semantic (cleanup):                00:00:00.003785580 ( 476.78MB)
Semantic (recursive struct check): 00:00:00.000530665 ( 476.78MB)
Codegen (crystal):                 00:00:01.421233198 ( 588.78MB)
Codegen (bc+obj):                  00:00:10.021156172 ( 588.78MB)
Codegen (linking):                 00:00:00.418528598 ( 588.78MB)
dsymutil:                          00:00:00.060007620 ( 604.78MB)

Codegen (bc+obj):
 - no previous .o files were reused

real	0m13.675s
user	0m14.764s
sys	0m1.351s

This PR:

$ time bin/crystal build foo.cr -s
Using compiled compiler at `.build/crystal'
Parse:                             00:00:00.000118621 (   0.25MB)
Semantic (top level):              00:00:00.335138769 ( 141.16MB)
Semantic (new):                    00:00:00.000748274 ( 141.16MB)
Semantic (type declarations):      00:00:00.042147838 ( 141.16MB)
Semantic (abstract def check):     00:00:00.004128044 ( 141.16MB)
Semantic (ivars initializers):     00:00:00.006740879 ( 141.16MB)
Semantic (cvars initializers):     00:00:00.061190217 ( 157.16MB)
Semantic (main):                   00:00:00.638093375 ( 269.34MB)
Semantic (cleanup):                00:00:00.003363172 ( 269.34MB)
Semantic (recursive struct check): 00:00:00.000472708 ( 269.34MB)
Codegen (crystal):                 00:00:00.736523215 ( 317.59MB)
Codegen (bc+obj):                  00:00:04.330238197 ( 317.59MB)
Codegen (linking):                 00:00:00.240875165 ( 317.59MB)
dsymutil:                          00:00:00.046648033 ( 317.59MB)

Codegen (bc+obj):
 - no previous .o files were reused

real	0m6.508s
user	0m7.554s
sys	0m0.811s

That's probably because String.interpolation(...) for the same types is cached by the compiler and then reused every time.

Another test that could be done is generating many string interpolations with different types and number of arguments. But note that the above program is 100_000 different string interpolations and it doesn't take that much time, so I think this change is safe for performance (and could actually be a performance improvement).

[asterite]

But note that the above program is 100_000 different string interpolations and it doesn't take that much time

With that I also mean that it's very unlikely that a program will have that many different string interpolations, so this benchmark is like the worse case ever.

[asterite]

Also note that there are 4069 different interpolations across this entire repository, so I think we are still safe with this change :-)

[asterite]

I pushed another commit where #{value} with value a String returns just that string, not a copy, as we discussed above.

Let's do that, mark it as a breaking change, and see if it affects anyone. If so, we can revert it to return a copy.

[RX14]

I did some timings of compiling the spec suite and compiler in a spreadsheet.

[asterite]

Interesting. So it's slower? Let me try it too

[asterite]

I just did that experiment too. Compiling the compiler is slightly faster (around 2 seconds). The all_specs is more or less the same (0.2 seconds slower). I think there's enough flux compiling things that there's no way to draw a conclusion here other than maybe "the changes here don't significantly affect compile-time performance".

[RX14]

@asterite did you use a release mode compiler when testing compile times? Did you clean the compiler cache?

My results were 17 sigma for 7% slower on compiling the compiler.

But it's really interesting that it seems the slowdown and speedup is heavily based on the code being compiled... I don't want this reverted, though. I just find it interesting.

I'd really like to automate speed-testing over time but no time :)

[asterite]

yes

[rdp]

Looks nice.
I think I might be getting some weird backtraces after this commit on OS X, but only with --release FWIW:

test file:

$ cat bad4.cr
def go1
  go2
end

def go2
  raise "boom"
end
go1

$ ./bin/crystal run --release bad4.cr
Using compiled compiler at `.build/crystal'
Unhandled exception: boom (Exception)
  from src/string.cr:4539:0 in 'go2' # THIS LINE
  from bad4.cr:2:3 in 'go1'
  from bad4.cr:9:1 in '__crystal_main'
  from src/crystal/main.cr:97:5 in 'main'

:)