-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 Span to fill List<T> in more ToList scenarios #86796
Conversation
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsAdds span-based filling of
Secondary improvements:
Fixes #80760
|
Thanks. Can you share some benchmarks that demonstrate these improvements are meaningful, in particular in the places that are requiring a lot of code to be added, like in AppendPrepend1Iterator? |
Upon more careful review, the added complications in Append/Prepend were mostly not worth it. Missing the optimizations in I'll add benchmarks on some of the other changes shortly. BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22621.1776) PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
|
Here are some benchmarks of some other cases. BenchmarkDotNet=v0.13.2.2052-nightly, OS=Windows 11 (10.0.22621.1776) PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:EnableUnsafeBinaryFormatterSerialization=true IterationTime=250.0000 ms
|
src/libraries/System.Linq/src/System/Linq/AppendPrepend.SpeedOpt.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Linq/src/System/Linq/AppendPrepend.SpeedOpt.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Linq/src/System/Linq/DefaultIfEmpty.SpeedOpt.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Linq/src/System/Linq/DefaultIfEmpty.SpeedOpt.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Are there any further changes required on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @brantburnett, apologies for the delay in review. Before we get this merged, any chance you could remove the singleton optimization since it doesn't appear to be contributing substantially?
Sure, I'll try to get it done today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Adds span-based filling of
List<T>
during calls toToList
usingCollectionsMarshal.SetCount
to the following additional scenarios:ToList
of aUnion
orDistinct
ToList
of aLookup<TKey, TElement>
including cases within groupingToList
of aSelect
projection against anIPartition
(i.e. sorted)ToList
of anOrderedEnumerable
returning a single element (i.e.Skip
/Take
applied to a sorted list for a single result)ToList
ofAppend
andPrepend
scenarios, mostly multiple appends/prependsSecondary improvements:
ToArray
of an emptyLookup<TKey, TElement>
now usesArray.Empty
ToList
of multiplePrepend
operations no longer allocates an intermediate arrayFixes #80760