Avoid creating dependent slots in NullableWalker#48920
Conversation
dbf31c3 to
72f8720
Compare
72f8720 to
ca5a752
Compare
| { | ||
| #if REPORT_MAX | ||
| int slots = _variableSlot.Count; | ||
| if (slots > _maxSlots.Item2) |
There was a problem hiding this comment.
Item2 [](start = 34, length = 5)
nit: name tuple elements?
|
Just curious, what gave you an indication this was a problem to investigate in the first place? |
| private SharedWalkerState SaveSharedState() => | ||
| new SharedWalkerState( | ||
| #if REPORT_MAX | ||
| private static (Symbol, int) _maxSlots; |
There was a problem hiding this comment.
private static should have the s_ prefix attached
|
|
||
| private SharedWalkerState SaveSharedState() => | ||
| new SharedWalkerState( | ||
| #if REPORT_MAX |
There was a problem hiding this comment.
Consider
#if !DEBUG
#error Need REPORT_MAX and DEBUG
#endif| (containingType is null || AccessCheck.IsSymbolAccessible(member, containingType, ref discardedUseSiteDiagnostics))) | ||
| { | ||
| int childSlot = GetOrCreateSlot(member, slot, true); | ||
| int childSlot = GetOrCreateSlot(member, slot, forceSlotEvenIfEmpty: true, createIfMissing: false); |
There was a problem hiding this comment.
true [](start = 88, length = 4)
It feels inconsistent passing true here and false for the next argument
There was a problem hiding this comment.
I'll address in a subsequent PR.
| "); | ||
| c.VerifyDiagnostics( | ||
| ); | ||
| // (13,17): warning CS8602: Dereference of a possibly null reference. |
There was a problem hiding this comment.
// (13,17): warning CS8602: Dereference of a possibly null reference. [](start = 16, length = 69)
Description of the PR sounds like a pure optimization, but there is a behavior change. Was this behavior change one of the goals of the PR, or is this just a fall out of the change that are willing to live with?
There was a problem hiding this comment.
The goal of the PR was to fix the perf issue, and the behavior change here fell out of that. That said, the old behavior for this scenario doesn't seem much better than the new behavior, and not worth the cost.
|
Done with review pass (iteration 2) |
Avoid creating dependent slots in
NullableWalker.MarkDependentSlotsNotNull.When compiling src/Compilers/CSharp/Portable, reduces elapsed time by ~13% and reduces total allocations by ~20%.