-
Notifications
You must be signed in to change notification settings - Fork 777
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
Lower interpolation into a call to concat #16556
Conversation
❗ Release notes required
|
a44ceed
to
e9e4f4b
Compare
79260a2
to
fdea8a5
Compare
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
fdea8a5
to
a60c558
Compare
Also, if anyone is curious about same benchmarks but with server GC:
With optimization:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good and clear PR :)
A question for my education. Ideologically, how is this optimization different from the string optimization that Optimizer does in MakeOptimizedSystemStringConcatCall?
tests/FSharp.Compiler.ComponentTests/Language/InterpolatedStringsTests.fs
Outdated
Show resolved
Hide resolved
tests/FSharp.Compiler.ComponentTests/EmittedIL/StringFormatAndInterpolation.fs
Show resolved
Hide resolved
Co-authored-by: Petr <psfinaki@users.noreply.github.com>
I am not really familiar with it, but on a quick glance, it is something else entirely - it (makes and) optimizes a call to |
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. |
Alright then, thanks for the explanation there :) |
/run fantomas |
Co-authored-by: psfinaki <5451366+psfinaki@users.noreply.github.com>
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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):
Results with the flag set (with optimization):
Checklist