Dictionary/List code clean up/formatting (C#7) by benaadams · Pull Request #17196 · dotnet/coreclr

benaadams · 2018-03-24T23:08:33Z

and some mild asm improves

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

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

Top method improvements by size (bytes):
        -603 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this (29 methods)
        -451 : System.Private.CoreLib.dasm - Dictionary`2:CopyTo(ref,int):this (29 methods)
        -376 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
        -370 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
         -71 : System.Private.CoreLib.dasm - KeyCollection:CopyTo(ref,int):this (31 methods)
         -71 : System.Private.CoreLib.dasm - ValueCollection:CopyTo(ref,int):this (31 methods)
         -33 : System.Private.CoreLib.dasm - List`1:RemoveRange(int,int):this (23 methods)
         -21 : System.Private.CoreLib.dasm - List`1:System.Collections.IList.Add(ref):int:this (27 methods)
         -18 : System.Private.CoreLib.dasm - List`1:set_Item(int,struct):this (12 methods)
          -9 : System.Private.CoreLib.dasm - List`1:Clear():this (27 methods)
          -3 : System.Private.CoreLib.dasm - List`1:set_Item(int,ref):this
          -2 : System.Private.CoreLib.dasm - PinnableBufferCache:TrimFreeListIfNeeded():bool:this
          -2 : System.Private.CoreLib.dasm - List`1:set_Item(int,ushort):this (2 methods)
          -2 : System.Private.CoreLib.dasm - List`1:set_Item(int,ubyte):this (2 methods)
          -1 : System.Private.CoreLib.dasm - EventSource:InitializeProviderMetadata():this
          -1 : System.Private.CoreLib.dasm - RuntimeAssembly:GetForwardedTypes():ref:this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,short):this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,byte):this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,bool):this

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

benaadams · 2018-03-25T00:02:09Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

benaadams · 2018-03-25T04:55:20Z

baseline failures https://github.com/dotnet/coreclr/issues/17205

Discovering: System.Security.SecureString.Tests
Discovered:  System.Security.SecureString.Tests
Starting:    System.Security.SecureString.Tests
Assertion Failed
The new chunk should be larger than the one it is replacing.

   at System.Text.StringBuilder.set_Length(Int32 value)
   at System.Text.StringBuilder.Remove(Int32 startIndex, Int32 length)
   at System.Security.Tests.SecureStringTests.GrowAndContract_Small() in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.Security.SecureString/tests/SecureStringTests.cs:line 426
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass46_1.<<InvokeTestMethodAsync>b__1>d.MoveNext()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at ...

jkotas · 2018-03-25T14:40:08Z

                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
            }

+            _version++;


Why move the version increment here? It is a potential breaking change.

jkotas · 2018-03-25T14:40:42Z

                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
            }

+            _version++;


jkotas · 2018-03-25T14:41:37Z

        // 
        public void Insert(int index, T item)
        {
+            _version++;


Same here. Won't comment on all places.

benaadams · 2018-03-25T15:06:46Z

Why move the version increment here? It is a potential breaking change.

Along same lines as "Prevent concurrent use corruption from causing infinite loops" #16991

At the moment the version will only update if Insert/Resize or Remove actually does something; however the concurrent use was still there (if the version change breaks an enumeration); so should detect it before the "somethings gone really bad" state?

Can back it out (especially since this was mostly cosmetic changes)

benaadams · 2018-03-25T20:51:38Z

Dropped the behaviour changing version moves

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

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

Top method improvements by size (bytes):
        -603 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.ICollection.CopyTo(ref,int):this (29 methods)
        -451 : System.Private.CoreLib.dasm - Dictionary`2:CopyTo(ref,int):this (29 methods)
        -376 : System.Private.CoreLib.dasm - ValueCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
        -370 : System.Private.CoreLib.dasm - KeyCollection:System.Collections.ICollection.CopyTo(ref,int):this (30 methods)
         -71 : System.Private.CoreLib.dasm - KeyCollection:CopyTo(ref,int):this (31 methods)
         -71 : System.Private.CoreLib.dasm - ValueCollection:CopyTo(ref,int):this (31 methods)
         -21 : System.Private.CoreLib.dasm - List`1:System.Collections.IList.Add(ref):int:this (27 methods)
         -18 : System.Private.CoreLib.dasm - List`1:set_Item(int,struct):this (12 methods)
          -9 : System.Private.CoreLib.dasm - List`1:Clear():this (27 methods)
          -3 : System.Private.CoreLib.dasm - List`1:set_Item(int,ref):this
          -2 : System.Private.CoreLib.dasm - PinnableBufferCache:TrimFreeListIfNeeded():bool:this
          -2 : System.Private.CoreLib.dasm - List`1:set_Item(int,ushort):this (2 methods)
          -2 : System.Private.CoreLib.dasm - List`1:set_Item(int,ubyte):this (2 methods)
          -1 : System.Private.CoreLib.dasm - EventSource:InitializeProviderMetadata():this
          -1 : System.Private.CoreLib.dasm - RuntimeAssembly:GetForwardedTypes():ref:this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,short):this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,byte):this
          -1 : System.Private.CoreLib.dasm - List`1:set_Item(int,bool):this

18 total methods with size differences (18 improved, 0 regressed), 17263 unchanged.

jkotas · 2018-03-25T21:08:39Z

Thanks.

Here is example that I was concerned about:

var d = new Dictionary<string,string>();
d.Add("a", "b");
d.Add("c", "d");
foreach (var e in d)
{
    if (d.Remove(e.Key + "Sibling")) break;
}

This works perfectly fine today, but it would start throwing exceptions with the version increments moved.

benaadams · 2018-03-25T21:10:27Z

Here is example that I was concerned about:

Ah... yeah that's valid

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>

stephentoub · 2018-03-28T19:57:05Z

                if (entries[i].hashCode >= 0)
                {
-                    array[index++] = new KeyValuePair<TKey, TValue>(entries[i].key, entries[i].value);
+                    array[index + i] = new KeyValuePair<TKey, TValue>(entries[i].key, entries[i].value);


This is a breaking change and needs to be reverted. Let's say there were 2 entries in the dictionary and then 1 was removed, so for example Count == 1, entries[0].hashCode == -1, entries[1] == the item. This would previously copy the single entry to array[0]; now it's going to copy it to array[1], which a) is the wrong index and b) will blow up if a developer passed in an array sized to Count.
cc: @jkotas, @benaadams

due to the skip :-/

Give a few mins...

i always increases but there might not be an entry (due to hashCode < 0) so the index for the array shouldn't increase when there isn't an entry.

Right, and _count isn't the same as Count (Count is confusingly _count - _freeCount). Lots of System.Net.Http outerloop tests are failing as a result. Looks like we must also then have a Dictionary test hole that needs to be plugged.

_count isn't the same as Count (Count is confusingly _count - _freeCount). Lots of System.Net.Http outerloop tests are failing as a result. Looks like we must also then have a Dictionary test hole that needs to be plugged.

Because they should have failed with array out of bounds on CopyTo?

Because they should have failed with array out of bounds on CopyTo?

That's the failure mode hitting System.Net.Http, as CurlHandler uses CopyTo.

But if the output array is large enough to accommodate it, the bug could also manifest as a gap in the copied data.

jkotas · 2018-03-28T20:10:30Z


            set
            {
+                _version++;


This move of the _version++ should be also reverted.

danmoseley · 2018-03-29T20:22:20Z

Do we need a test for this also?

var d = new Dictionary<string,string>();
d.Add("a", "b");
d.Add("c", "d");
foreach (var e in d)
{
    if (d.Remove(e.Key + "Sibling")) break;
}

benaadams added 2 commits March 24, 2018 21:40

Dictionary/List code clean up/formatting (C#7)

fc91e3d

Reduce asm

c5bb5ba

jkotas approved these changes Mar 25, 2018

View reviewed changes

jkotas reviewed Mar 25, 2018

View reviewed changes

Revert behaviour change version moves

3971a59

benaadams force-pushed the dict-list-codeformatting branch from d755f40 to 3971a59 Compare March 25, 2018 20:41

jkotas merged commit 922c5a4 into dotnet:master Mar 25, 2018

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

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

01aa608

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

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

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

ec0ed94

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 26, 2018

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

981448c

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 26, 2018

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

851c4d5

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

Anipik pushed a commit to dotnet/corert that referenced this pull request Mar 26, 2018

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

c74e45e

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

jkotas pushed a commit to dotnet/corefx that referenced this pull request Mar 26, 2018

Dictionary/List code clean up/formatting (C#7) (dotnet/coreclr#17196)

e5bcd9a

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

stephentoub reviewed Mar 28, 2018

View reviewed changes

jkotas reviewed Mar 28, 2018

View reviewed changes

benaadams mentioned this pull request Mar 30, 2018

Use a non-contiguous Dictionary for tests dotnet/corefx#28643

Merged

benaadams deleted the dict-list-codeformatting branch April 2, 2018 05:59

lewurm mentioned this pull request Feb 1, 2019

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

Merged

This was referenced Jan 31, 2020

Failure: StringBuilder.set_Length: The new chunk should be larger than the one it is replacing. dotnet/runtime#10028

Closed

Extra test should be added for Dictionary dotnet/runtime#25737

Closed

Conversation

benaadams commented Mar 24, 2018

Uh oh!

benaadams commented Mar 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Mar 25, 2018

Uh oh!

benaadams commented Mar 25, 2018

Uh oh!

jkotas commented Mar 25, 2018

Uh oh!

benaadams commented Mar 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benaadams commented Mar 25, 2018 •

edited

Loading

jkotas Mar 25, 2018 •

edited

Loading

stephentoub Mar 28, 2018 •

edited

Loading