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

Remove span pinning associated with string.Create #78914

Merged
merged 3 commits into from Dec 8, 2022

Conversation

stephentoub
Copy link
Member

This is now possible with C# 11.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned stephentoub Nov 28, 2022
@jakobbotsch
Copy link
Member

jakobbotsch commented Nov 28, 2022

Keep in mind that this pattern introduces new address exposure, so it might be worth it to double check that it does not impact the preceding uses of the span in the same method significantly. If you want to be on the safe side you can create a copy of the span.

For example

[MethodImpl(MethodImplOptions.NoInlining)]
private static unsafe string Foo(ReadOnlySpan<char> chars)
{
    for (int i = 0; i < chars.Length; i++)
    {
        if (chars[i] >= 128)
            return null;
    }

    //return string.Create(chars.Length, (IntPtr)(&chars), static (s, c) =>
    //{
    //    for (int j = 0; j < s.Length; j++)
    //        s[j] = (*(ReadOnlySpan<char>*)c)[j];
    //});
    fixed (char* charsPtr = chars)
    {
        return string.Create(chars.Length, (IntPtr)charsPtr, static (s, c) =>
        {
            for (int j = 0; j < s.Length; j++)
                s[j] = ((char*)c)[j];
        });
    }
}

Codegen for the loop with the uncommented vs commented code on linux-x64:

 G_M27558_IG03:
-       mov      eax, esi
-       cmp      word  ptr [rdi+2*rax], 128
-       jae      G_M27558_IG09
-       inc      esi
-       cmp      esi, ebx
+       mov      rsi, bword ptr [rbp-28H]
+       mov      eax, edi
+       cmp      word  ptr [rsi+2*rax], 128
+       jae      G_M27558_IG07
+       inc      edi
+       cmp      edi, dword ptr [rbp-20H]
        jl       SHORT G_M27558_IG03

(Fixing this would be the "partial lifetime promotion" item in #76931)

@stephentoub
Copy link
Member Author

Thanks.

Codegen for the loop with the uncommented vs commented code on linux-x64:

Ugh, it reintroduces bounds checking on the prior loop? That's not something we can fix?

SharpLab

@stephentoub stephentoub force-pushed the stringcreatespanpinning branch 2 times, most recently from e772612 to a937b78 Compare December 5, 2022 20:38
@jakobbotsch
Copy link
Member

Ugh, it reintroduces bounds checking on the prior loop? That's not something we can fix?

Not easily. Address exposure turns off lots of basic analyses and optimizations for the exposed local and we're very coarse grained about it today.

@stephentoub
Copy link
Member Author

@jakobbotsch, does it look ok now? Thanks.

@stephentoub stephentoub merged commit c8f6012 into dotnet:main Dec 8, 2022
@stephentoub stephentoub deleted the stringcreatespanpinning branch December 8, 2022 11:45
@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants