Skip to content

lower interpolated strings to string concatenation where possible#22595

Closed
KrisVandermotten wants to merge 7 commits intodotnet:masterfrom
KrisVandermotten:interpolation
Closed

lower interpolated strings to string concatenation where possible#22595
KrisVandermotten wants to merge 7 commits intodotnet:masterfrom
KrisVandermotten:interpolation

Conversation

@KrisVandermotten
Copy link
Copy Markdown
Contributor

@KrisVandermotten KrisVandermotten commented Oct 9, 2017 •
edited
Loading

This PR handles #22594.

Customer scenario

The user uses string interpolation where concatenation could be used.

Risk

This PR modifies code generation of interpolated strings, but only in the case where all fill-ins are strings without alignment or format specifiers.

Performance impact

Code generation for interpolated strings will be somewhat slower. The generated code will be significantly faster in the case where all fill-ins in the interpolated string are strings, without alignment or format specifiers. In all other cases, the new implementation has one additional loop over the parts with no memory allocations in it, fairly simple logic and early exit as soon it is determined that the code gen optimization cannot be done.

@KrisVandermotten
Copy link
Copy Markdown
Contributor Author

KrisVandermotten commented Oct 9, 2017

@dotnet-bot retest windows_release_vs-integration_prtest please

Therzok
Therzok reviewed Oct 9, 2017
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs
.maxstack 3
.locals init (string V_0, //x
string V_1) //y
IL_0000: ldstr ""goodbye""
Copy link
Copy Markdown
Contributor

@Therzok Therzok Oct 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goodbye cannot be found in the concat version.

Copy link
Copy Markdown
Contributor Author

@KrisVandermotten KrisVandermotten Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is has been optimized away, because it is not used (as it is overwritten by null later).

@KrisVandermotten KrisVandermotten mentioned this pull request Oct 10, 2017
Closed
@KrisVandermotten
Copy link
Copy Markdown
Contributor Author

KrisVandermotten commented Oct 23, 2017

@gafter It seems that you wrote most of the original code. Would you like to take a look?

@jcouv jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 24, 2017
@KrisVandermotten
Copy link
Copy Markdown
Contributor Author

KrisVandermotten commented Oct 28, 2017

I found a bug. The lowering is incorrect for $"{s}".

There are three cases:

  • s is constant with value null: we should lower to ""
  • s is constant with non-null value: we should lower to s
  • s is not constant: we should lower to s ?? ""

I will commit a fix as soon as possible.

@KrisVandermotten
Copy link
Copy Markdown
Contributor Author

KrisVandermotten commented Nov 3, 2017 •
edited
Loading

The bugs have been fixed.

Given the following declarations:

const string constantabc = "abc";
const string constantnull = null;

With /o, the following methods compile to the following IL:

string M1() => $"";

IL_0000: ldstr ""
IL_0005: ret

string M2() => $"abc";

IL_0000: ldstr "abc"
IL_0005: ret

string M3() => $"{constantabc}";

IL_0000: ldstr "abc"
IL_0005: ret

string M4() => $"{constantnull}";

IL_0000: ldstr ""
IL_0005: ret

string M5() => $"{constantabc}{constantnull}";

IL_0000: ldstr "abc"
IL_0005: ret

string M6(string a) => $"{a}";

IL_0000: ldarg.1
IL_0001: dup
IL_0002: brtrue.s IL_000a
IL_0004: pop
IL_0005: ldstr ""
IL_000a: ret

string M7(string a) => $"a: {a}";

IL_0000: ldstr "a: "
IL_0005: ldarg.1
IL_0006: call string [mscorlib]System.String::Concat(string, string)
IL_000b: ret

string M8(string a, string b) => $"{a + b}";

IL_0000: ldarg.1
IL_0001: ldarg.2
IL_0002: call string [mscorlib]System.String::Concat(string, string)
IL_0007: ret

string M9(string a, string b) => $"{a}{b}";

IL_0000: ldarg.1
IL_0001: ldarg.2
IL_0002: call string [mscorlib]System.String::Concat(string, string)
IL_0007: ret

string M10(string a, string b) => $"a: {a}, b: {b}";

IL_0000: ldstr "a: "
IL_0005: ldarg.1
IL_0006: ldstr ", b: "
IL_000b: ldarg.2
IL_000c: call string [mscorlib]System.String::Concat(string, string, string, string)
IL_0011: ret

string M11(object a) => $"{a}";

IL_0000: ldstr "{0}"
IL_0005: ldarg.1
IL_0006: call string [mscorlib]System.String::Format(string, object)
IL_000b: ret

string M12(object a) => $"a: {a}";

IL_0000: ldstr "a: {0}"
IL_0005: ldarg.1
IL_0006: call string [mscorlib]System.String::Format(string, object)
IL_000b: ret

string M13(string a) => $"{{{a}}}";

IL_0000: ldstr "{"
IL_0005: ldarg.1
IL_0006: ldstr "}"
IL_000b: call string [mscorlib]System.String::Concat(string, string, string)
IL_0010: ret

string M14(string a) => "a:" + $" {a}";

IL_0000: ldstr "a: "
IL_0005: ldarg.1
IL_0006: call string [mscorlib]System.String::Concat(string, string)
IL_000b: ret

@KrisVandermotten KrisVandermotten mentioned this pull request Nov 4, 2017
Closed
@KrisVandermotten
Copy link
Copy Markdown
Contributor Author

KrisVandermotten commented Nov 9, 2017 •
edited
Loading

@gafter Can you please review this PR? Any and all feedback would be most welcome.

@KrisVandermotten KrisVandermotten mentioned this pull request Nov 10, 2017
Closed
@lechu445 lechu445 mentioned this pull request Feb 12, 2018
Closed
@wiz0u wiz0u mentioned this pull request Feb 14, 2018
Closed
@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 19, 2018

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!

@jaredpar jaredpar closed this Apr 19, 2018
@jaredpar jaredpar added the Resolution-Expired The request is closed due to inactivity under our contribution review policy. label Apr 19, 2018
@KrisVandermotten KrisVandermotten mentioned this pull request May 3, 2018
Merged
@KrisVandermotten KrisVandermotten deleted the interpolation branch June 2, 2018 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Therzok Therzok Therzok left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-already-signed Community The pull request was submitted by a contributor who is not a Microsoft employee. Resolution-Expired The request is closed due to inactivity under our contribution review policy.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@KrisVandermotten @jaredpar @Therzok @dnfclas @jcouv