Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement string interpolation as a call to String.interpolation #8400

Merged

Conversation

@asterite
Copy link
Member

asterite commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Member

bcardiff commented Oct 29, 2019

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

# In this case the implementation just returns the same string value.
#
# NOTE: there should never be a need to call this method instead of using string interpolation.
def self.interpolation(value : String)

This comment has been minimized.

Copy link
@straight-shoota

straight-shoota Oct 29, 2019

Member

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

This comment has been minimized.

Copy link
@asterite

asterite Oct 29, 2019

Author Member

You are right

@wontruefree

This comment has been minimized.

Copy link
Contributor

wontruefree commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 29, 2019

@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

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Oct 29, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 29, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 29, 2019

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.

Copy link
Contributor

bew left a comment

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

src/string.cr Outdated Show resolved Hide resolved
src/string.cr Show resolved Hide resolved
@vlazar

This comment has been minimized.

Copy link
Contributor

vlazar commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

konovod commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Contributor

vlazar commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 30, 2019

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.

@r00ster91

This comment has been minimized.

Copy link
Contributor

r00ster91 commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 30, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Oct 30, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Oct 30, 2019

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

@yxhuvud

This comment has been minimized.

Copy link
Contributor

yxhuvud commented Nov 1, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 1, 2019

@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

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 1, 2019

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

This comment has been minimized.

Copy link
Member

RX14 commented Nov 8, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 8, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 8, 2019

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

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 8, 2019

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

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Nov 8, 2019

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.

Copy link
Member

bcardiff left a comment

Although we still need #8400 (comment)

Co-Authored-By: Benoit de Chezelles <bew@users.noreply.github.com>
@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Nov 8, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 8, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 8, 2019

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

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 9, 2019

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

@asterite asterite added this to the 0.32.0 milestone Nov 9, 2019
@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 9, 2019

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.

@asterite asterite force-pushed the asterite:string-interpolation-expansion branch from d2c9aac to 28ac4fd Nov 9, 2019
@asterite asterite merged commit b300056 into crystal-lang:master Nov 9, 2019
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: test_darwin Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
ci/circleci: test_preview_mt Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite deleted the asterite:string-interpolation-expansion branch Nov 9, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Nov 9, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 9, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 9, 2019

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

This comment has been minimized.

Copy link
Member

RX14 commented Nov 9, 2019

@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

This comment has been minimized.

Copy link
Member Author

asterite commented Nov 9, 2019

yes

@rdp

This comment has been minimized.

Copy link
Contributor

rdp commented Nov 19, 2019

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'

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.