Lower interpolation into a call to concat by abonie · Pull Request #16556 · dotnet/fsharp

abonie · 2024-01-19T14:36:02Z

Description

Optimization that lowers string interpolation into a call to concat iff there are at most 4 string parts and all fill expressions are strings.

Fixes #16247

Benchmarks

Run a benchmark like in this gist with and without the feature flag set.

Results without the flag (no optimization):


BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3155/22H2/2022Update/SunValley2)
11th Gen Intel Core i7-11850H 2.50GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.300-preview.24101.10
  [Host]     : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI DEBUG
  DefaultJob : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Mean	Error	StdDev	Gen0	Allocated
SingleVariable	70.49 ns	1.214 ns	1.136 ns	0.0107	136 B
JustLiterals	145.61 ns	1.443 ns	1.350 ns	0.0215	272 B
Variables	144.05 ns	0.959 ns	0.897 ns	0.0215	272 B
Function	116.73 ns	1.557 ns	1.381 ns	0.0191	240 B
LongString	363.35 ns	2.838 ns	2.654 ns	0.4330	5432 B

Results with the flag set (with optimization):


BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3155/22H2/2022Update/SunValley2)
11th Gen Intel Core i7-11850H 2.50GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.300-preview.24101.10
  [Host]     : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI DEBUG
  DefaultJob : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Mean	Error	StdDev	Median	Gen0	Allocated
SingleVariable	0.0004 ns	0.0013 ns	0.0011 ns	0.0000 ns	-	-
JustLiterals	12.0562 ns	0.0719 ns	0.0673 ns	12.0500 ns	0.0032	40 B
Variables	11.9894 ns	0.0810 ns	0.0718 ns	12.0101 ns	0.0032	40 B
Function	12.2912 ns	0.1201 ns	0.1124 ns	12.3161 ns	0.0032	40 B
LongString	206.0906 ns	2.4311 ns	2.0301 ns	206.5404 ns	0.4158	5216 B

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated

github-actions · 2024-01-19T14:37:03Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description

src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.300.md

LanguageFeatures.fsi docs/release-notes/.Language/preview.md

src/Compiler/Checking/CheckExpressions.fs

Sanity check that lowering to concat does not break these simple cases

Initial attempt with many TODOs, also not sure whether it should be done in checking, but it seems that later we would have to again parse the string (since CheckExpressions is going from AST version of an interpolated string to a sprintf call basically)

Cannot really optimize this way if width and other flags are specified. Typed interpolated expressions should be possible to support, but skipping them for now (TODO).

E.g. $"{x}{y}" has 5 string parts, including 3 empty strings

There were false positives before

abonie · 2024-02-15T16:47:10Z

Also, if anyone is curious about same benchmarks but with server GC:
No optimization:

Method	Mean	Error	StdDev	Gen0	Allocated
SingleVariable	78.08 ns	1.021 ns	0.905 ns	0.0005	136 B
JustLiterals	157.44 ns	2.675 ns	2.502 ns	0.0010	272 B
Variables	151.16 ns	1.498 ns	1.401 ns	0.0010	272 B
Function	125.50 ns	1.239 ns	1.099 ns	0.0010	240 B
LongString	641.34 ns	2.861 ns	2.536 ns	0.0219	5432 B

With optimization:

Method	Mean	Error	StdDev	Median	Gen0	Allocated
SingleVariable	0.0064 ns	0.0103 ns	0.0096 ns	0.0000 ns	-	-
JustLiterals	14.8223 ns	0.1345 ns	0.1258 ns	14.8525 ns	0.0001	40 B
Variables	14.1441 ns	0.1240 ns	0.1099 ns	14.1558 ns	0.0001	40 B
Function	14.2070 ns	0.1159 ns	0.1028 ns	14.2344 ns	0.0001	40 B
LongString	508.0675 ns	6.9382 ns	6.1506 ns	507.0075 ns	0.0210	5216 B

…rp-to-concat

psfinaki

Good and clear PR :)

A question for my education. Ideologically, how is this optimization different from the string optimization that Optimizer does in MakeOptimizedSystemStringConcatCall?

src/Compiler/FSComp.txt

tests/FSharp.Compiler.ComponentTests/Language/InterpolatedStringsTests.fs

tests/FSharp.Compiler.ComponentTests/EmittedIL/StringFormatAndInterpolation.fs

src/Compiler/Checking/CheckFormatStrings.fs

Co-authored-by: Petr <psfinaki@users.noreply.github.com>

abonie · 2024-02-20T12:20:34Z

A question for my education. Ideologically, how is this optimization different from the string optimization that Optimizer does in MakeOptimizedSystemStringConcatCall?

I am not really familiar with it, but on a quick glance, it is something else entirely - it (makes and) optimizes a call to System.String.Concat (while this change will make it so that there is a call to Concat in the first place)

psfinaki · 2024-02-20T13:24:05Z

I am not really familiar with it, but on a quick glance, it is something else entirely - it (makes and) optimizes a call to System.String.Concat (while this change will make it so that there is a call to Concat in the first place)

Well yeah I just noticed that it also deals with <5 parts so I wonder if your PR doesn't somehow supersede that thing.

abonie · 2024-02-20T13:36:52Z

Well yeah I just noticed that it also deals with <5 parts so I wonder if your PR doesn't somehow supersede that thing.

It doesn't supersede it for sure. MakeOptimizedSystemStringConcatCall deals with situations where there was a call to Concat in the source code AFAICT. And the reason for the 5 parts thing is that there are optimized overloads for Concat for 2-4 arguments. That's why in my PR if there are more than 4 parts, we don't even want to do Concat, it would be slower.

psfinaki · 2024-02-20T13:39:16Z

Alright then, thanks for the explanation there :)

src/Compiler/Checking/CheckExpressions.fs

psfinaki · 2024-02-22T11:25:25Z

/run fantomas

Co-authored-by: psfinaki <5451366+psfinaki@users.noreply.github.com>

github-actions · 2024-02-22T11:26:36Z

Ran fantomas: https://github.com/dotnet/fsharp/actions/runs/8003712144

src/Compiler/Checking/CheckExpressions.fs

abonie · 2024-02-22T15:50:22Z

/azp run

azure-pipelines · 2024-02-22T15:50:37Z

Azure Pipelines successfully started running 2 pipeline(s).

T-Gro reviewed Jan 22, 2024

View reviewed changes

src/Compiler/Checking/CheckExpressions.fs Outdated Show resolved Hide resolved

T-Gro approved these changes Jan 22, 2024

View reviewed changes

abonie force-pushed the str-interp-to-concat branch 3 times, most recently from a44ceed to e9e4f4b Compare February 5, 2024 19:28

abonie force-pushed the str-interp-to-concat branch 3 times, most recently from 79260a2 to fdea8a5 Compare February 14, 2024 16:45

abonie added 12 commits February 15, 2024 17:02

Add unit tests for interp strings with string expr

191950d

Sanity check that lowering to concat does not break these simple cases

Add IL test for lowering to concat

0bb2323

Do not optimize when format specifiers are there

1804afe

Cannot really optimize this way if width and other flags are specified. Typed interpolated expressions should be possible to support, but skipping them for now (TODO).

Adjust expected length of codegen

12e231e

Filter out empty string parts

7b7b4c7

E.g. $"{x}{y}" has 5 string parts, including 3 empty strings

Detect format specifiers better

03200ae

There were false positives before

Refactor, improve regex

9d7419d

Unrelated indentation fix in parser

8f3d2b2

Use language feature flag

af418a3

FSComp.txt auto-update

467e876

Add release notes

a60c558

abonie force-pushed the str-interp-to-concat branch from fdea8a5 to a60c558 Compare February 15, 2024 16:18

abonie changed the title ~~[WIP] Lower interpolation into a call to concat~~ Lower interpolation into a call to concat Feb 15, 2024

abonie and others added 3 commits February 16, 2024 14:36

Add langversion flag to some unit tests

d299d70

Merge branch 'main' of https://github.com/abonie/fsharp into str-inte…

b9c3bc4

…rp-to-concat

Merge branch 'main' into str-interp-to-concat

47e390e

abonie marked this pull request as ready for review February 16, 2024 16:18

abonie requested a review from a team as a code owner February 16, 2024 16:18

abonie requested a review from T-Gro February 16, 2024 18:32

vzarytovskii approved these changes Feb 20, 2024

View reviewed changes

psfinaki reviewed Feb 20, 2024

View reviewed changes

Fix typo in src/Compiler/FSComp.txt

f3c42b9

Co-authored-by: Petr <psfinaki@users.noreply.github.com>

Update .xlf and undo change to CheckFormatStrings

629812e

abonie and others added 2 commits February 20, 2024 16:08

Add a test, undo change in CheckFormatString

3bcb745

Merge branch 'main' into str-interp-to-concat

fe5059a

psfinaki approved these changes Feb 21, 2024

View reviewed changes

src/Compiler/Checking/CheckExpressions.fs Outdated Show resolved Hide resolved

src/Compiler/Checking/CheckExpressions.fs Outdated Show resolved Hide resolved

abonie added 3 commits February 21, 2024 18:46

Refactor based on review suggestions

cbb7277

Add more IL tests

b4f0bf3

Add comments lost in refactoring

eb62292

Automated command ran: fantomas

ea6c6ec

Co-authored-by: psfinaki <5451366+psfinaki@users.noreply.github.com>

Merge branch 'main' into str-interp-to-concat

22fe9f2

psfinaki approved these changes Feb 22, 2024

View reviewed changes

vzarytovskii reviewed Feb 22, 2024

View reviewed changes

src/Compiler/Checking/CheckExpressions.fs Show resolved Hide resolved

Merge branch 'main' into str-interp-to-concat

1eb4ad4

abonie enabled auto-merge (squash) February 22, 2024 16:02

abonie merged commit 2de1f68 into dotnet:main Feb 22, 2024

dotnet-bot mentioned this pull request Feb 29, 2024

[Automated] PRs inserted in VS build main-34628.35 #16788

Closed

brianrourkeboll mentioned this pull request Jun 8, 2024

FS-1135 implementation - random functions for collections #17277

Merged

3 tasks

Numpsy mentioned this pull request Jul 23, 2024

Building expecto test apps as NativeAOT haf/expecto#498

Open

brianrourkeboll mentioned this pull request Dec 4, 2024

Can't prefix format specifier with '%' in interpolated strings #18103

Closed

ncave mentioned this pull request Jan 6, 2025

Fable 5.0.0-alpha.4 JSX.jsx problem with not recognizing interpolated string fable-compiler/Fable#3999

Open

Conversation

abonie commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Benchmarks

Checklist

Uh oh!

github-actions bot commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Release notes required

Uh oh!

Uh oh!

abonie commented Feb 15, 2024

Uh oh!

psfinaki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abonie commented Feb 20, 2024

Uh oh!

psfinaki commented Feb 20, 2024

Uh oh!

abonie commented Feb 20, 2024

Uh oh!

psfinaki commented Feb 20, 2024

Uh oh!

Uh oh!

Uh oh!

psfinaki commented Feb 22, 2024

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

Uh oh!

abonie commented Feb 22, 2024

Uh oh!

azure-pipelines bot commented Feb 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

abonie commented Jan 19, 2024 •

edited

Loading

github-actions bot commented Jan 19, 2024 •

edited

Loading