Optimize GreenNode.CreateList#48568
Conversation
|
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. |
333fred
left a comment
There was a problem hiding this comment.
Overall looks good, a couple of issues.
|
Here are some performance figures. The currently committed version is
|
|
Based on those numbers, looks like we should stick with IROL and not use List. Or, we have an overload for both or something (but htat seems like a lot of complexity). |
|
Is the |
|
The only types every passed in are |
|
I've just tested this but the results aren't beyond some error, so I doubt it makes a difference. |
What did you test? |
|
Having both an overload for List & IROList null => null,
List<TFrom> list => GreenNode.CreateList(list, select),
IReadOnlyList list => GreenNode.CreateList(list, select),
_ => GreenNode.CreateList(enumerable.ToList(), select) |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 9). @dotnet/roslyn-compiler for a second review of this community perf improvement.
|
I've updated the original post to show current numbers. |
| default: { | ||
| var array = new ArrayElement<GreenNode>[list.Count]; | ||
| for (int i = 0; i < array.Length; i++) | ||
| array[i].Value = select(list[i]); |
There was a problem hiding this comment.
📝 Even though list was already declared as having non-null GreenNode items, the new behavior here is slightly different for the case where a null value appeared. I'm not sure if that's a problem or not.
| var list = nodes.ToArray(); | ||
|
|
||
| switch (list.Length) | ||
| public static GreenNode? CreateList<TFrom>(IReadOnlyList<TFrom> list, Func<TFrom, GreenNode> select) |
There was a problem hiding this comment.
💭 This does make me wonder if we could somehow change this to only have one overload that takes ReadOnlySpan<TFrom>. Perhaps for the future...
There was a problem hiding this comment.
I also thought so. Note however that the only case where this is used is from WithLeading/Trailing trivia, and in those cases it seems more beneficial to check for IROList. See the List/Array benchmark above, it's not Span, but it indicates less special = better. And if I'm not missing anything ROSpan is a rare thing to be provided. (we could look into special casing for Array/Memory/ROMemory/Span/ROSpan and call into the ROSpan overload in all those cases. Will try and maybe open another PR)
There was a problem hiding this comment.
And if I'm not missing anything ROSpan is a rare thing to be provided
ReadOnlySpan<T> cannot be cast to IEnumerable<T>, so to use this form all the callers would need to be updated to provide the correct type.
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
…nto optimize-greennode-createlist
|
Not sure why those tests failed. Seems like something timed out? |
|
Thanks @HurricanKai! |
This PR improves the performance of GreenNode.CreateList.
This is part of a larger goal to improve NormalizeWhitespace performance, the benchmark below tests NormalizeWhitespace performance of real-world generated C# source (~116k loc when normalized).
Benchmark:
Tested on Net5.0 RC2 with the Benchmark runtime being .NET Core 3.1.8
I also investigated a couple of other approaches (ImmutableArray + SelectAsArray, GetEnumerator directly) but this one came out on top.
Thanks for the help @333fred and @CyrusNajmabadi 😄
Benchmark Files (drop into IdeCoreBenchmarks for testing):
normalization-sample.txt
NormalizeWhitespaceBenchmark.cs