Make some Dictionary code smaller

[mikedn]

Store a reference to the relevant entry at the start of the loop to avoid indexing entries multiple times.
This avoids some redundant range checks in Remove and allows the elimination of a duplicate index++ in Enumerator.MoveNext.

Saves ~7KB of code.

Fixes #10883

[mikedn]

jit-diff summary:

X64:
Total bytes of diff: -6853 (-0.20 % of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
       -6853 : System.Private.CoreLib.dasm (-0.20 % of base)

1 total files with size differences (1 improved, 0 regressed).

Top method improvements by size (bytes):
       -2111 : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (117 methods)
       -1047 : System.Private.CoreLib.dasm - Dictionary`2:Remove(ref):bool:this (11 methods)
        -827 : System.Private.CoreLib.dasm - Dictionary`2:Remove(ref,byref):bool:this (8 methods)
        -562 : System.Private.CoreLib.dasm - Dictionary`2:Remove(struct):bool:this (7 methods)
        -558 : System.Private.CoreLib.dasm - Dictionary`2:Remove(struct,byref):bool:this (6 methods)

13 total methods with size differences (13 improved, 0 regressed).

X86:
Total bytes of diff: -8182 (-0.29 % of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
       -8182 : System.Private.CoreLib.dasm (-0.29 % of base)

1 total files with size differences (1 improved, 0 regressed).

Top method improvements by size (bytes):
       -2058 : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (117 methods)
       -1371 : System.Private.CoreLib.dasm - Dictionary`2:Remove(ref):bool:this (11 methods)
        -822 : System.Private.CoreLib.dasm - Dictionary`2:Remove(ref,byref):bool:this (8 methods)
        -811 : System.Private.CoreLib.dasm - Dictionary`2:Remove(int):bool:this (7 methods)
        -787 : System.Private.CoreLib.dasm - Dictionary`2:Remove(struct):bool:this (7 methods)

13 total methods with size differences (13 improved, 0 regressed).

[mikedn]

Using Remove to remove all keys from a Dictionary<int, int> containing 10000 keys shows a 10% improvement in performance. Dictionary enumeration performance isn't affected.

This approach can be used in other places (FindEntry for example) but the code size savings are much smaller. Besides, it's not clear why the JIT can't eliminate these range checks and it would better to investigate this a bit from the JIT side rather than hacking the Dictionary code now.

[omariom]

TryInsert could benefit from that too.

[mikedn]

TryInsert could benefit from that too

It's in the same boat as FindEntry, not so great code size savings. If there was no risk of code size (and performance) regressions I'd change all such loops in the Dictionary code but unfortunately this kind of change can also make things worse.

[omariom]

I played with it when prerelease Roslyn bits. There were small improvements in perf and the size of the loop. But I checked it on the full framework and for primitive keys only.

[stephentoub]

Did this change from the for loop to a while impact the code gen?

[mikedn]

Nope, they all look like while loops to JIT and it transforms them into do-while loops.

Dictionary.Remove diff: https://gist.github.com/mikedn/ccf5673a57ef7182c961b4e387ab5c0e

[stephentoub]

Ok, that's what I'd have guessed, thanks. So this change was just for consistency?

[mikedn]

You mean the change from for to while? It's because entries[i] was used in the iterator expression of the for statement and there entry is not available.

I could have kept for and only move the iterator expression to the end of the for body. But it's not a typical for anyway so I changed it to while.

[stephentoub]

Right. Ok.

[mikedn]

While I'm at it here's a MoveNext diff: https://gist.github.com/mikedn/b7fa2f3d31becba5f92f3ea768028250

[jkotas]

Thanks!