-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Use single argument overload for [start..]
of string
or (ReadOnly)Span
#74643
base: main
Are you sure you want to change the base?
Conversation
2fe693f
to
7324eeb
Compare
rebased to e8bae2f, the latest merge commit where the CI succeeds. |
All perf changes need to be accompanied by benchmarks showing their impact, just "produces smaller il" is not enough. The linked issue has benchmarks from 4 years ago; that's both very out of date, and the benchmarking code has a lot of unrelated operations in it. |
https://github.com/tats-u/SubstringSliceBench For
For
|
I'll have to compare the internal code of |
@stephentoub, a penny for your thoughts on this optimization? |
I've not looked at the PR, but from my perspective addressing it would be nice to have: |
I agree with you. |
Should I optimize for |
I'ma fan of this sort of optimization. It means that people can use the more modern/idiomatic patterns, without worry about a perf cliff. |
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Fred Silberberg <frsilb@microsoft.com>
Update: eventually didn't occur |
roslyn/src/Compilers/CSharp/Test/Semantic/Semantics/SpanStackSafetyTests.cs Lines 1725 to 1727 in cad8698
In the current change, this diagnostic doesn't seem to come up.
|
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_IndexerAccess.cs
Outdated
Show resolved
Hide resolved
Also, it looks like there are test failures. |
Co-authored-by: Fred Silberberg <frsilb@microsoft.com>
I knew that but the CI insists that I hadn't fixed the places where I fixed in e2b3f40. |
I saw the console output, but the ILs in Expected are those before that commit. |
All existing unit tests now pass, but I couldn't understand what the following in Rebuild tests means:
|
Uses the single argument overloads of
string.Substring
and(ReadOnly)Span.Slice
instead of 2 arguments ones for lowering[start..]
.This change shortens ILs and x64 assemblies.
Fixes #47629
TODO:
Memory
Slice
methodFailsILVerify.WithILVerifyMessage
stloc.
ldloc.