Skip to content

Optimize PreComputeTupleConstructor - #11230

Merged
cartermp merged 2 commits into
dotnet:mainfrom
kerams:tuple-ctor
Mar 20, 2021
Merged

Optimize PreComputeTupleConstructor#11230
cartermp merged 2 commits into
dotnet:mainfrom
kerams:tuple-ctor

Conversation

@kerams

@keramskerams commented Mar 13, 2021

Copy link
Copy Markdown
Contributor
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen 7 3700X, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.200-preview.21077.7
[Host] : .NET Core 5.0.2 (CoreCLR 5.0.220.61120, CoreFX 5.0.220.61120), X64 RyuJIT DEBUG
DefaultJob : .NET Core 5.0.2 (CoreCLR 5.0.220.61120, CoreFX 5.0.220.61120), X64 RyuJIT
MethodMeanErrorStdDevMedianGen 0Gen 1Gen 2Allocated
New3Part134.50 ns3.313 ns9.768 ns129.21 ns0.0842--704 B
Old3Part1,758.37 ns19.991 ns18.699 ns1,757.57 ns0.1755--1480 B
New1Part14.62 ns0.330 ns0.309 ns14.68 ns0.0115--96 B
Old1Part283.67 ns2.794 ns2.614 ns282.73 ns0.0162--136 B

Tested against

letthreePart=(("yup",1s),2,3,4,5,6,7,8,9,10,11, Some 12,13,"nope",17,18, ValueSome 19)letonePart=("yup",1s)

There's one little problem though. When values of the wrong type are passed to the precomputed constructor, InvalidCastException is now thrown instead of ArgumentException (MakeTuple still throws this as well). I'm not sure if this matters that much (I suspect a similar change sneaked into #9784), but if it does, I could theoretically emit a runtime check throwing the old exception here at the cost of a small performance hit.

@cartermp

Copy link
Copy Markdown
Contributor

This is technically a breaking change given the change in exception that's thrown, but I think that's a very minor thing given the perf benefits.

@cartermp

Copy link
Copy Markdown
Contributor

Could you update the failing test?

@cartermpcartermp added this to the 16.10 milestone Mar 20, 2021
@cartermp
cartermp merged commit b4c5af6 into dotnet:mainMar 20, 2021
@kerams
kerams deleted the tuple-ctor branch March 20, 2021 19:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@kerams@cartermp@vzarytovskii