Small tweaks to Dict asm size by benaadams · Pull Request #17096 · dotnet/coreclr

benaadams · 2018-03-21T16:35:34Z

Shrinks from #17076

Dictionary``2:Clear() reduction from moving _version++ out of the if was unexpected

Total bytes of diff: -513 (-0.01% 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):
        -513 : System.Private.CoreLib.dasm (-0.01% of base)

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

Top method improvements by size (bytes):
        -319 : System.Private.CoreLib.dasm - Dictionary`2:Clear():this (29 methods)
         -24 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,struct,ubyte):bool:this (4 methods)
         -22 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,struct,ubyte):bool:this (2 methods)
         -20 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,ref,ubyte):bool:this (3 methods)
         -18 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,int,ubyte):bool:this (3 methods)
         -18 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,ref,ubyte):bool:this (3 methods)
         -12 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,ref,ubyte):bool:this (2 methods)
         -10 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,ubyte,ubyte):bool:this
         -10 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,int,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,ref,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,bool,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,bool,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,short,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ushort,ref,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(short,long,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,int,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,ubyte,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,long,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,ushort,ubyte):bool:this

19 total methods with size differences (19 improved, 0 regressed), 17254 unchanged.

/cc @jkotas

benaadams · 2018-03-21T16:45:45Z

Clear has two returns with the _version++ in the if

Pre

; Lcl frame size = 40
G_M52189_IG01:
       push     rdi
       push     rsi
       sub      rsp, 40
       mov      rsi, rcx
G_M52189_IG02:
       mov      edi, dword ptr [rsi+56]
       test     edi, edi
       jle      SHORT G_M52189_IG04
       mov      rcx, gword ptr [rsi+8]
       mov      r8d, dword ptr [rcx+8]
       xor      edx, edx
       call     Array:Clear(ref,int,int)
       xor      ecx, ecx
       mov      dword ptr [rsi+56], ecx
       mov      dword ptr [rsi+60], -1
       mov      dword ptr [rsi+64], ecx
       inc      dword ptr [rsi+68]
       mov      rcx, gword ptr [rsi+16]
       mov      r8d, edi
       xor      edx, edx
       lea      rax, [(reloc)]
G_M52189_IG03:
       add      rsp, 40
       pop      rsi
       pop      rdi
       rex.jmp  rax
G_M52189_IG04:
       add      rsp, 40
       pop      rsi
       pop      rdi
       ret      
; Total bytes of code 81, prolog size 9 for method Dictionary`2:Clear():this

Post

; Lcl frame size = 40
G_M52189_IG01:
       push     rdi
       push     rsi
       sub      rsp, 40
       mov      rsi, rcx
G_M52189_IG02:
       mov      edi, dword ptr [rsi+56]
       test     edi, edi
       jle      SHORT G_M52189_IG03
       mov      rcx, gword ptr [rsi+8]
       mov      r8d, dword ptr [rcx+8]
       xor      edx, edx
       call     Array:Clear(ref,int,int)
       xor      ecx, ecx
       mov      dword ptr [rsi+56], ecx
       mov      dword ptr [rsi+60], -1
       mov      dword ptr [rsi+64], ecx
       mov      rcx, gword ptr [rsi+16]
       mov      r8d, edi
       xor      edx, edx
       call     Array:Clear(ref,int,int)
G_M52189_IG03:
       inc      dword ptr [rsi+68]
G_M52189_IG04:
       add      rsp, 40
       pop      rsi
       pop      rdi
       ret      
; Total bytes of code 70, prolog size 6 for method Dictionary`2:Clear():this

jkotas · 2018-03-21T16:49:25Z

src/mscorlib/shared/System/Collections/Generic/Dictionary.cs

-                _version++;
                Array.Clear(_entries, 0, count);
            }
+            _version++;


Why is this an improvement? I would expect that it prevents tailcall for Array.Clear. Should the version increment be done as the first thing, similar to insert?

Doesn't reduce the size, though keeping the tail call have moved now the two Clears together at end so it can just run through cache setting them before making the calls out

; Lcl frame size = 40 G_M52189_IG01: push rdi push rsi sub rsp, 40 mov rsi, rcx G_M52189_IG02: mov edi, dword ptr [rsi+56] test edi, edi jle SHORT G_M52189_IG04 xor r8d, r8d mov dword ptr [rsi+56], r8d mov dword ptr [rsi+60], -1 mov dword ptr [rsi+64], r8d inc dword ptr [rsi+68] mov rcx, gword ptr [rsi+8] mov r8d, dword ptr [rcx+8] xor edx, edx call Array:Clear(ref,int,int) mov rcx, gword ptr [rsi+16] mov r8d, edi xor edx, edx lea rax, [(reloc)] G_M52189_IG03: add rsp, 40 pop rsi pop rdi rex.jmp rax G_M52189_IG04: add rsp, 40 pop rsi pop rdi ret ; Total bytes of code 84, prolog size 9 for method Dictionary`2:Clear():this

jkotas

Thanks

benaadams · 2018-03-21T17:02:51Z

Total bytes of diff: -107 (0.00% 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):
        -107 : System.Private.CoreLib.dasm (0.00% of base)

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

Top method regessions by size (bytes):
          87 : System.Private.CoreLib.dasm - Dictionary`2:Clear():this (29 methods)

Top method improvements by size (bytes):
         -24 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,struct,ubyte):bool:this (4 methods)
         -22 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,struct,ubyte):bool:this (2 methods)
         -20 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,ref,ubyte):bool:this (3 methods)
         -18 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,int,ubyte):bool:this (3 methods)
         -18 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,ref,ubyte):bool:this (3 methods)
         -12 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,ref,ubyte):bool:this (2 methods)
         -10 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,ubyte,ubyte):bool:this
         -10 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(struct,int,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,ref,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,bool,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,bool,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(long,short,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ushort,ref,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(short,long,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,int,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(int,ubyte,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,long,ubyte):bool:this
          -6 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,ushort,ubyte):bool:this

19 total methods with size differences (18 improved, 1 regressed), 17254 unchanged.

What's better tail call or code size? (I assume Clear isn't called frequently?)

jkotas · 2018-03-21T17:14:32Z

Codesize is better for Clear

benaadams · 2018-03-21T17:22:18Z

Reverted to original diff as in summary

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

* Small tweaks to Dict asm size (dotnet/coreclr#17096) Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Moving Span APIs that allow skipping visibility checks to MemoryMarshal (#17087) Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Improve DateTime{Offset} "r" and "o" formatting performance (#17092) Two main changes: 1. Rewrote the formatting to use span, only to then discover that we already had almost exactly the same implementation in Utf8Formatter. As that one had some extra optimizations around JIT behaviors, I ported that over instead. 2. Avoided [ThreadStatic] lookups unless necessary. ToString/TryFormat for "o"/"O" improve by ~2.5x. ToString/TryFormat for "r"/"R" improve by ~3x. Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Rename {Try}Read/WriteMachineEndian to just {Try}Read/Write (#17106) Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Fix incorrect array dereference. (dotnet/coreclr#17113) Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com> * Move Span APIs that allow skipping visibility checks to MemoryMarshal * Rename {Try}Read/WriteMachineEndian to just {Try}Read/Write * Update calls to BinaryPrimitives.ReadMachineEndian * Rename calls to ReadMachineEndian in System.Memory perf tests. * Add ApiCompatBaseline for UWP NETNative * Add to ApiCompatBaseline for UWP NETNative netstandard20

…completely, which only showed modest size improvement * Removing the _version increment from Clear() entirely to bring it in line with the behavior in Remove() and to keep size gains [tfs-changeset: 1714543]

…tely, which only showed modest size improvement * Removing the _version increment from Clear() entirely to bring it in line with the behavior in Remove() and to keep size gains [tfs-changeset: 1714543] Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

…completely, which only showed modest size improvement * Removing the _version increment from Clear() entirely to bring it in line with the behavior in Remove() and to keep size gains [tfs-changeset: 1714543] Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

…which only showed modest size improvement * Removing the _version increment from Clear() entirely to bring it in line with the behavior in Remove() and to keep size gains [tfs-changeset: 1714543] Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

danmoseley · 2018-09-21T16:58:56Z

This was reverted, because

Executing the following code snippet
            Dictionary<string, string> dictionary = new Dictionary<string, string>();
            var valuesEnum = dictionary.GetEnumerator();
                                                          
            dictionary.Clear();
            valuesEnum.MoveNext();
produces an InvalidOperationException. The enumerator is invalidated regardless of the fact that the underlying dictionary collection has not changed.

This snippet:
            Dictionary<string, string> dictionary = new Dictionary<string, string>();
            dictionary.Add("key", "value_0");
            var valuesEnum = dictionary.GetEnumerator();
            dictionary.TryAdd("key", "value_1"); // this should not invalidate the enumerator

            valuesEnum.MoveNext();

Also invalidates the iterator, even though TryAdd fails, returns false and does not modify the dictionary values.

danmoseley · 2018-09-21T17:09:36Z

@jkotas I assume we should add tests for the above, since apparently we didn't have any, or is this now covered by changes that went into dotnet/corefx@eca3f8d#diff-c0be16052840e601127fc0a5e7b5c3af ?

jkotas · 2018-09-21T17:14:42Z

Right, @A-And is working on adding tests for this.

Small tweaks to Dict asm size

c476122

benaadams mentioned this pull request Mar 21, 2018

Strong detection for List/Dictionary concurrent use in Debug #17076

Closed

jkotas reviewed Mar 21, 2018

View reviewed changes

jkotas approved these changes Mar 21, 2018

View reviewed changes

benaadams force-pushed the dict-code-size branch from 38c86e2 to c476122 Compare March 21, 2018 17:21

jkotas merged commit 06f6f87 into dotnet:master Mar 21, 2018

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Mar 21, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

6f37c3b

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Mar 21, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

783f76a

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

ahsonkhan pushed a commit to dotnet/corert that referenced this pull request Mar 21, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

f8edf2e

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Mar 22, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

84c1aad

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Mar 22, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

e9e600e

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Mar 22, 2018

Small tweaks to Dict asm size (dotnet/coreclr#17096)

3c9464a

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

benaadams deleted the dict-code-size branch March 28, 2018 04:42

A-And mentioned this pull request Sep 22, 2018

Add TryAdd and Clear regression tests dotnet/corefx#32407

Merged

lewurm mentioned this pull request Feb 1, 2019

[2018-08] Bump corert mono/mono#12721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small tweaks to Dict asm size#17096

Small tweaks to Dict asm size#17096
jkotas merged 1 commit intodotnet:masterfrom
benaadams:dict-code-size

benaadams commented Mar 21, 2018 •

edited

Loading

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

jkotas Mar 21, 2018

Uh oh!

benaadams Mar 21, 2018

Uh oh!

jkotas left a comment

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

jkotas commented Mar 21, 2018

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

danmoseley commented Sep 21, 2018

Uh oh!

danmoseley commented Sep 21, 2018

Uh oh!

jkotas commented Sep 21, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

benaadams commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

jkotas Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

benaadams Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

jkotas commented Mar 21, 2018

Uh oh!

benaadams commented Mar 21, 2018

Uh oh!

danmoseley commented Sep 21, 2018

Uh oh!

danmoseley commented Sep 21, 2018

Uh oh!

jkotas commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benaadams commented Mar 21, 2018 •

edited

Loading

jkotas commented Sep 21, 2018 •

edited

Loading