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

Replace unsafe code with Span<char> #6784

Merged
merged 6 commits into from Feb 1, 2019

Conversation

Projects
None yet
@meziantou
Copy link
Contributor

meziantou commented Jan 17, 2019

Replace unsafe code for a stackalloc with Span<char>

@meziantou meziantou requested a review from Tratcher as a code owner Jan 17, 2019

@meziantou meziantou force-pushed the meziantou:use-span branch 2 times, most recently from d47c403 to 4d7d1dd Jan 17, 2019

@meziantou meziantou force-pushed the meziantou:use-span branch from 4d7d1dd to 9a49069 Jan 17, 2019

@gfoidl

This comment has been minimized.

Copy link
Contributor

gfoidl commented Jan 17, 2019

Can you also try to use string.Create, so that there's no need for the stack allocation and copying to the string either? (This should result in quite faster code, and IMHO cleaner code).

(FYI: aspnet/KestrelHttpServer#2579 was too unsafe to get taken)

@Tratcher

This comment has been minimized.

Copy link
Member

Tratcher commented Jan 17, 2019

@sebastienros for benchmarks

@Tratcher

This comment has been minimized.

Copy link
Member

Tratcher commented Jan 17, 2019

A more interesting question is why we have three copies of this method.

@meziantou meziantou requested a review from jkotalik as a code owner Jan 17, 2019

@meziantou

This comment has been minimized.

Copy link
Contributor Author

meziantou commented Jan 17, 2019

@gfoidl I didn't know about string.Create! The last commit uses this method.

@meziantou meziantou force-pushed the meziantou:use-span branch 2 times, most recently from 5560118 to 8f60240 Jan 17, 2019

@Eilon Eilon added the area-servers label Jan 18, 2019

@meziantou meziantou force-pushed the meziantou:use-span branch from eee8d40 to a528701 Jan 18, 2019

@meziantou

This comment has been minimized.

Copy link
Contributor Author

meziantou commented Jan 18, 2019

I've made a benchmark: https://gist.github.com/meziantou/7ab15c70f7e9fc0b55dec4b074fb3209

BenchmarkDotNet=v0.11.3, OS=Windows 10.0.16299.785 (1709/FallCreatorsUpdate/Redstone3)
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical and 6 physical cores
Frequency=3507502 Hz, Resolution=285.1032 ns, Timer=TSC
.NET Core SDK=2.2.101
  [Host] : .NET Core 2.2.0 (CoreCLR 4.6.27110.04, CoreFX 4.6.27110.04), 64bit RyuJIT
  Core   : .NET Core 2.2.0 (CoreCLR 4.6.27110.04, CoreFX 4.6.27110.04), 64bit RyuJIT

Job=Core  Runtime=Core  
Method Mean Error StdDev Ratio Gen 0/1k Op Gen 1/1k Op Gen 2/1k Op Allocated Memory/Op
original 28.49 ns 0.0724 ns 0.0677 ns 1.00 0.0089 - - 56 B
span 30.07 ns 0.0569 ns 0.0475 ns 1.06 0.0089 - - 56 B
latest commit 15.98 ns 0.1588 ns 0.1407 ns 0.56 0.0089 - - 56 B
@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Jan 19, 2019

/AzurePipelines run AspNetCore-ci

@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Jan 19, 2019

Success! 2 created and queued.

@davidfowl
Copy link
Member

davidfowl left a comment

We have way too many copies of this code.

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Jan 19, 2019

/AzurePipelines run AspNetCore-ci

@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Jan 19, 2019

Success! 2 created and queued.

@NimaAra

This comment has been minimized.

Copy link

NimaAra commented Jan 20, 2019

I did an analysis of this last year HERE.

@sebastienros

This comment has been minimized.

Copy link
Member

sebastienros commented Jan 20, 2019

@NimaAra when I saw this PR it immediately reminded me of your blog post, to the point that I assumed the author was you, didn't even try to check. Awesome blog articles BTW.

@Daniel-Svensson

This comment has been minimized.

Copy link

Daniel-Svensson commented Jan 21, 2019

Great article @NimaAra 👍 might be worth a comment that he ordering is there to eliminate bounds checks, in case anybody else are unsure why.

@meziantou

This comment has been minimized.

Copy link
Contributor Author

meziantou commented Jan 21, 2019

I'm not sure assigning from 12 to 0 or 0 to 12 change something here. From my benchmark (code), the results are very close (16.41ns vs 16.87ns). Maybe the JIT is better than before in avoiding bouds checks.

@gfoidl

This comment has been minimized.

Copy link
Contributor

gfoidl commented Jan 21, 2019

Maybe the JIT is better than before in avoiding bouds checks

The JIT produces bound checks for "0 to 12", but I assume the branch predictor of your (computer's) CPU does a good job.
For "12 to 0" only one bound check for "12" needs to be emitted. Aside from good branch prediction this results in smaller code (which is almost always preferable).

Ideally the JIT would handle this for us...

@Suchiman

This comment has been minimized.

Copy link
Contributor

Suchiman commented Jan 22, 2019

@gfoidl

Ideally the JIT would handle this for us…

Except that technically, this would be a breaking change. I'm not sure if it's in the realm of too esoteric, but if the JIT detects the highest range check in a basic block and moves it first, eliminating the other ones, then no data would be modified in an out of bounds situation and the exception will be raised immediately. Normally, you would be able to modify e.g. the first 10 indexes before you run into an IndexOutOfRangeException at index 11.

@jkotalik jkotalik merged commit f1b24cc into aspnet:master Feb 1, 2019

1 check passed

license/cla All CLA requirements met.
Details

@meziantou meziantou deleted the meziantou:use-span branch Feb 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.