Reduce Pattern matching allocations #1590
Conversation
|
@benaadams, |
nkolev92
requested review from
emgarten,
nkolev92,
zhili1208,
alpaix,
mishra14 and
jainaashish
|
Has issues |
benaadams
force-pushed
the
PatternExpression.Initialize
branch
from
3dac48b
to
3f7163c
Compare
|
Missed an |
|
Saves 8,292 MB on delete |
|
@benaadams is this working for you now? I can re-run the tests on this branch |
|
Yes; I'd missed a not sign before, rookie mistake - hours to track down. |
|
|
||
| public Dictionary<string, object> Properties | ||
| { | ||
| get => _properties ?? CreateDictionary(); |
There was a problem hiding this comment.
Why not get => _properties ?? (_properties = new Dictionary<string, object>());? I think that's a quite common pattern. Or would that make the getter too big to get inlined later?
There was a problem hiding this comment.
@halter73 told me off for doing that once; now is habit 😄
Smaller for inline without the new; though unlikely to be significant.
There was a problem hiding this comment.
I guess every repo/team/project has different guidelines :) I was just curious if there is a reason not to use the pattern I personally find quite nice.
|
✔️ all tests are passing for this branch Thanks for the fix @benaadams! These patterns get hit going through every file from every nupkg during restore, this will be a great improvement 🚀 |
| @@ -144,6 +142,21 @@ public virtual bool TryLookup(string name, PatternTable table, out object value) | |||
| return false; | |||
| } | |||
|
|
|||
| private static bool ContainsSlash(string name) | |||
There was a problem hiding this comment.
IndexOfAny with a char array is a multi stage process where it first creates a probabilistic map then scans for possible matches before checking for actual match. Which ends up being a little overkill for just 2 item arrays; and its faster just to check for the two chars.
There was a problem hiding this comment.
Came across when looking into a perf issue with MVC; which was also 2 delimiters aspnet/Mvc#5362
There was a problem hiding this comment.
Did we get a bug on this? If not, I'm going to file one - I'm seeing the same thing.
There was a problem hiding this comment.
Nope never filed anything; was when I was working exclusively upstream; before I got caught by the slippery slope and started contributing to coreclr
There was a problem hiding this comment.
Saves 8,292 MB on delete
roslyn\Binaries\Obj+ runnuget restore Roslyn.sln -NoCacheResolves NuGet/Home#5668 (Except
SubStrings used forTokenSegment)Resolves NuGet/Home#5665 (Except
TokenSegment.TryMatchcallsstring.Substring)Contributes to NuGet/Home#5664 (
PatternExpression.Match)@sharwell is this more what you meant? Specifically benaadams@3dac48b