Use safe stackalloc where possible #1742

VSadov · 2017-09-07T01:21:35Z

No description provided.

VSadov · 2017-09-07T01:22:10Z

src/System.Text.Formatting/System/Text/Parsing/SequenceParser.cs

                    }
                }
            }

            // for invariant culture, we should never reach this point, as invariant uint text is never longer than 127 bytes. 
            // I left this code here, as we will need it for custom cultures and possibly when we shrink the stack allocated buffer.
-            combinedSpan = bufferSequence.ToSpan();
-            if (!PrimitiveParser.InvariantUtf8.TryParseUInt64(first.Span, out value, out consumed)) {


Was 'first.Span' a bug here?

I am not sure but it seems like it given combinedSpan is assigned and then not used.
cc @KrzysztofCwalina

I would not be shocked if this codepath never executed (as the comment implies)

VSadov · 2017-09-07T01:24:02Z

@KrzysztofCwalina @ahsonkhan @shiftylogic - please review.

VSadov · 2017-09-07T01:24:35Z

@stephentoub @jaredpar - FYI. Using safe stackalloc in CoreFxLab.

ahsonkhan · 2017-09-07T02:05:03Z

@dotnet-bot test Innerloop OSX10.12 Debug Build and Test

ahsonkhan · 2017-09-07T02:06:27Z

src/System.Buffers.Experimental/System/Buffers/BufferExtensions.cs

            // make 'remainder' formally stack-referring or we won't be able to reference stack data later
-            ReadOnlySpan<byte> remainder = stackalloc byte[0]; 
+            var remainder = true ? new ReadOnlySpan<byte>() : stackalloc byte[0];


nit: change from var to ReadOnlySpan, esp since in the future, the r.h.s would look like stackalloc byte[0];

Will do. I was not sure if mentioning the type twice would be too verbose.

ahsonkhan · 2017-09-07T02:11:43Z

src/System.IO.Pipelines.Text.Primitives/ReadableBufferExtensions.cs

+            // TODO: "ReadOnlySpan<byte> textSpan = stackalloc byte[0]" would fit better here, 
+            //       but it emits substandard IL, see https://github.com/dotnet/roslyn/issues/21952
+            //
+            // make 'textSpan' formally stack-referring or we won't be able to reference stack data later


I do not understand this comment.

What does it mean by 'formally stack-referring'. Also, shouldn't it read:
make 'textSpan' formally stack-referring or we will be able to reference stack data later

If textSpan is a stackalloc variable, we can't reference it outside the method, correct? The comment seems to suggest that if we don't make it 'stack-referring', we won't be able to reference it later, which has the opposite meaning.

If we do not assign anything, 'textSpan' will default to be a returnable variable. We will be able to return it from the method, but will not be able to assign stackalloc'd spans. That is what you normally want, but not in this case. Here we need to initialize to something stackallocated to intentionally "taint" the variable as "possibly referring to stack data".

'stackalloc byte[0]' would be a nice choice since it is basically the same as 'default(Span)', but formally stack-allocated. However we do a poor job with optimizing such trivial case, so I am using a more verbose, but reliable, way to essentially assign a default span, while forcing the variable be classified as stack-referring.

I will try to change the comment to be more clear.

ahsonkhan

LGTM

davidfowl · 2017-09-07T02:18:34Z

This is pretty nice cleanup

stephentoub · 2017-09-07T03:35:57Z

src/System.Buffers.Experimental/System/Buffers/BufferExtensions.cs

+                Span<byte> combined = combinedBufferLength < 128 ?
+                                        stackalloc byte[combinedBufferLength] :
+                                        // TODO (pri 3): I think this could be eliminated by chunking values
+                                        combined = new byte[combinedBufferLength];


Is this "combined =" on purpose or a mistake?

typo, it is a redundant assignment

KrzysztofCwalina · 2017-09-07T14:59:24Z

src/System.Azure.Experimental/System/Azure/CosmosDbAuthorizationHeader.cs

-                var pBuffer = stackalloc byte[AuthenticationHeaderBufferSize];
-                buffer = new Span<byte>(pBuffer, AuthenticationHeaderBufferSize);
-            }
+            Span<byte> buffer = stackalloc byte[AuthenticationHeaderBufferSize];


KrzysztofCwalina · 2017-09-07T15:53:42Z

src/System.Buffers.Experimental/System/Buffers/BufferExtensions.cs

-                    // TODO (pri 3): I think this could be eliminated by chunking values
-                    combined = new byte[combinedBufferLength];
-                }
+                var combined = combinedBufferLength < 128 ?


I think we should settle on the following idiom:

Span<byte> buffer = size < 128 ? stackalloc byte[size] : new byte[size];

.. as opposed to assigning to var and then having to cast.

As it stands now the dominant type of a ternary expression is selected from one of consequence or alternative branches.

If you have a span as a type of either of the branches, it will dominate. Otherwise compiler faces an ambiguity since no dominant type can be chosen.

I will bring this up to language folks.
There are some tradeoffs that we can have around the natural type of stackalloc.

KrzysztofCwalina · 2017-09-07T15:55:59Z

src/System.Buffers.Experimental/System/Buffers/BufferExtensions.cs

+            // Assign 'remainder' to something formally stack-referring.
+            // The default classification is "returnable, not referring to stack", we want the opposite in this case.
+            ReadOnlySpan<byte> remainder = true ? new ReadOnlySpan<byte>() : stackalloc byte[0];
+            Span<byte> stackSpan = stackalloc byte[stackLength];


Will this be fixed before we ship?

Can we just use ReadOnlySpan.Empty here?

@KrzysztofCwalina - yes, will fix this before shipping. The trivial stackalloc byte[0] is emitted "correctly", but the IL is embarrassingly redundant, even worse after JIT-ting.

Optimizing this case in C# compiler backend will benefit existing code as well, so the the value vs. effort is very favorable here.

@shiftylogic - if you use ReadOnlySpan.Empty or any other not stack allocated data to initialize the span you are telling the compiler - "I want to be able to return this from the method or mix with other returnables, please make sure I do not refer to stack data by accident".
That is generally the case.

Here we have a relatively rare opposite - We are slicing through stack allocated data and store in this variable, but have no intent to return it.
stackalloc byte[0] will convey to the compiler - "please allow referring to stack data, make sure I can never return this from the method"

KrzysztofCwalina · 2017-09-07T16:04:48Z

Wow, this is such a groundbreaking change to our platform where features in the run-time, libraries, and compilers come together so nicely.

@raja-krishnan, @MeiChin-Tsai

omariom · 2018-01-21T19:12:00Z

@KrzysztofCwalina

Wow, this is such a groundbreaking change to our platform where features in the run-time, libraries, and compilers come together so nicely.

The most impressive was how quickly the combined efforts of all three teams made it a real thing. A year ago it was a prototype in CorefxLab.

VSadov added 2 commits September 6, 2017 14:42

Use safe stackalloc where possible

330a409

some more changes

1cc6053

dnfclas added the cla-already-signed label Sep 7, 2017

VSadov commented Sep 7, 2017

View reviewed changes

ahsonkhan reviewed Sep 7, 2017

View reviewed changes

ahsonkhan approved these changes Sep 7, 2017

View reviewed changes

stephentoub reviewed Sep 7, 2017

View reviewed changes

VSadov force-pushed the stackalloc branch from 3ba95db to 461a6db Compare September 7, 2017 05:45

CR feedback

502debe

VSadov force-pushed the stackalloc branch from 461a6db to 502debe Compare September 7, 2017 05:50

KrzysztofCwalina reviewed Sep 7, 2017

View reviewed changes

KrzysztofCwalina merged commit 0339860 into dotnet:master Sep 7, 2017

alrz mentioned this pull request Sep 21, 2017

Discussion: consider target-type when the compiler can't determine the type of a ternary dotnet/csharplang#877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use safe stackalloc where possible #1742

Use safe stackalloc where possible #1742

VSadov commented Sep 7, 2017

VSadov Sep 7, 2017

ahsonkhan Sep 7, 2017

KrzysztofCwalina Sep 7, 2017

VSadov commented Sep 7, 2017

VSadov commented Sep 7, 2017

ahsonkhan commented Sep 7, 2017

ahsonkhan Sep 7, 2017

VSadov Sep 7, 2017

ahsonkhan Sep 7, 2017 •

edited

Loading

VSadov Sep 7, 2017

ahsonkhan left a comment

davidfowl commented Sep 7, 2017

stephentoub Sep 7, 2017

VSadov Sep 7, 2017

KrzysztofCwalina Sep 7, 2017

KrzysztofCwalina Sep 7, 2017

VSadov Sep 7, 2017

KrzysztofCwalina Sep 7, 2017

shiftylogic Sep 7, 2017

VSadov Sep 7, 2017

VSadov Sep 7, 2017

KrzysztofCwalina commented Sep 7, 2017

omariom commented Jan 21, 2018

Use safe stackalloc where possible #1742

Use safe stackalloc where possible #1742

Conversation

VSadov commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Sep 7, 2017

VSadov commented Sep 7, 2017

ahsonkhan commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

davidfowl commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina commented Sep 7, 2017

omariom commented Jan 21, 2018

ahsonkhan Sep 7, 2017 •

edited

Loading