Devirtualize TokenMap #47274

benaadams · 2020-08-31T04:40:01Z

Devirtualize Dictionary<T> (netcoreapp) in TokenMap by using a struct type
Devirtualize ConcurrentDictionary (net5 Reduce ConcurrentDictionary.TryGetValue overhead runtime#37081) in TokenMap by using a struct type
Reduce lock usage in TokenMap (only needed in AddItem, so should reduce contention)
Devirtualize HastSet<T>s (net5 Rewrite HashSet<T>'s implementation based on Dictionary<T>'s runtime#37180) in ReferenceIndexerBase
Pass object[] back directly rather than passing it as an IEnumerable<object> and then iterating it to copy it to an object[]

Was showing up as a hot path in when building Rosyln

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

src/Compilers/Core/Portable/IReferenceOrISignature.cs

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>

333fred · 2020-08-31T17:19:42Z

src/Compilers/Core/Portable/IReferenceOrISignature.cs

+    /// <summary>
+    /// Used to devirtualize Dictionary/HashSet for EqualityComparer{T}.Default
+    /// </summary>
+    internal struct IReferenceOrISignatureEquivalent : IEquatable<IReferenceOrISignatureEquivalent>


I'm not sure I understand why we would need both of these: it seems like just IReferenceOrISignatureEquivalent should be fine.

IReferenceOrISignature uses ReferenceEquals for equals and IReferenceOrISignatureEquivalent has a more complex equals, previously provided by MetadataEntityReferenceComparer.cs

The dictionary, hashtable and concurrentdictionary will devirtualize (and potentially inline) the equals for a struct when the Comparer is unspecified (if it also implements IEquatable) in .NET Core; whereas specifying a Comparer will be a slower interface call.

This would be great info to have in a comment in the file itself :)

In reply to: 480276103 [](ancestors = 480276103)

333fred · 2020-08-31T17:22:55Z

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

+                        items[(int)token] = item.AsObject();
+
+                        // Update the updated array reference before updating _count
+                        Volatile.Write(ref _items, items);


Why is this safe? It seems very likely to me that _items could have been changed by this point.

This is the only location _items is updated and it is done in a lock. The Volatile.Write is to ensure its not reordered with the write to _count; so it's safe to Volatile.Read them in reverse order in GetAllItems() since _count only ever increases

333fred · 2020-08-31T17:24:06Z

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

+            object[] items = Volatile.Read(ref _items);
+
+            // Return a right sized copy of the array
+            return (new ReadOnlySpan<object>(items, 0, count)).ToArray();


This doesn't seem particularly safe to me either.

Consider that AddItem does volatile writes in the following order: _items then _count. This method now reads them volatiley in the other order _count then _items. That means by the time we read _count here we know there are at least _count elements in the _items collection hence the array returned here will be well formed (no null values).

There is a subtle invariant change from the previous version of the code:

Before: If this method was entered when AddItem was executing then it would return the effect of completing the AddItem method

After: If this method was entered when AddItem was executing then it may or may not return the effect of completing the AddItem method.

Don't believe we care about maintaining that invariant.

333fred · 2020-08-31T17:26:23Z

Done review pass (commit 3). I'm very concerned by the threading in this PR: I think a full writeup of the strategy being taken here and the threading implications is necessary for review.

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

src/Compilers/Core/Portable/IReferenceOrISignature.cs

jaredpar · 2020-08-31T18:12:02Z

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

+            object[] items = Volatile.Read(ref _items);
+
+            // Return a right sized copy of the array
+            return (new ReadOnlySpan<object>(items, 0, count)).ToArray();


Consider that AddItem does volatile writes in the following order: _items then _count. This method now reads them volatiley in the other order _count then _items. That means by the time we read _count here we know there are at least _count elements in the _items collection hence the array returned here will be well formed (no null values).

There is a subtle invariant change from the previous version of the code:

Before: If this method was entered when AddItem was executing then it would return the effect of completing the AddItem method

After: If this method was entered when AddItem was executing then it may or may not return the effect of completing the AddItem method.

Don't believe we care about maintaining that invariant.

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

src/Compilers/Core/Portable/CodeGen/TokenMap.cs

src/Compilers/Core/Portable/IReferenceOrISignature.cs

benaadams · 2020-08-31T19:34:56Z

src/Compilers/Core/Portable/PEWriter/ReferenceIndexerBase.cs

@@ -46,7 +47,7 @@ public override void Visit(IEventDefinition eventDefinition)

        public override void Visit(IFieldReference fieldReference)
        {
-            if (!_alreadySeen.Add(fieldReference))
+            if (!_alreadySeen.Add(new IReferenceOrISignatureEquivalent(fieldReference)))


Shame can't do implicit conversion from an interface 😉

benaadams · 2020-09-05T04:29:56Z

REstarting CI

ghost · 2020-09-05T06:18:28Z

Hello @333fred!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

333fred · 2020-09-05T07:07:00Z

Thanks @benaadams!

Devirtualize TokenMap

b3afe69

benaadams requested a review from a team as a code owner August 31, 2020 04:40

333fred added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 31, 2020

Youssef1313 reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/CodeGen/TokenMap.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/CodeGen/TokenMap.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/CodeGen/TokenMap.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/IReferenceOrISignature.cs Outdated Show resolved Hide resolved

benaadams and others added 2 commits August 31, 2020 14:20

Apply suggestions from code review

7a9aee7

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>

Target typed new()

86dcb4b

333fred reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/CodeGen/TokenMap.cs Outdated Show resolved Hide resolved

jaredpar reviewed Aug 31, 2020

View reviewed changes

benaadams and others added 2 commits August 31, 2020 19:22

Apply suggestions from code review

7262313

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

Return ReadOnlySpan

658fbe3

jaredpar approved these changes Aug 31, 2020

View reviewed changes

Feedback

7753168

benaadams force-pushed the Devirtualize-TokenMap branch from 6126910 to 7753168 Compare August 31, 2020 18:46

benaadams commented Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/CodeGen/TokenMap.cs Outdated Show resolved Hide resolved

333fred approved these changes Aug 31, 2020

View reviewed changes

svick reviewed Aug 31, 2020

View reviewed changes

src/Compilers/Core/Portable/IReferenceOrISignature.cs Outdated Show resolved Hide resolved

Nullability changes

0d53ed8

benaadams commented Aug 31, 2020

View reviewed changes

benaadams closed this Sep 5, 2020

benaadams reopened this Sep 5, 2020

333fred added the auto-merge label Sep 5, 2020

ghost approved these changes Sep 5, 2020

View reviewed changes

333fred self-assigned this Sep 5, 2020

ghost merged commit b0827cf into dotnet:master Sep 5, 2020

ghost added this to the Next milestone Sep 5, 2020

benaadams deleted the Devirtualize-TokenMap branch September 5, 2020 09:13

dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devirtualize TokenMap #47274

Devirtualize TokenMap #47274

benaadams commented Aug 31, 2020 •

edited

Loading

333fred Aug 31, 2020 •

edited

Loading

benaadams Aug 31, 2020 •

edited

Loading

333fred Aug 31, 2020

333fred Aug 31, 2020

benaadams Aug 31, 2020

333fred Aug 31, 2020

jaredpar Aug 31, 2020

333fred commented Aug 31, 2020

jaredpar Aug 31, 2020

benaadams Aug 31, 2020

benaadams commented Sep 5, 2020

ghost commented Sep 5, 2020

ghost left a comment

333fred commented Sep 5, 2020

Devirtualize TokenMap #47274

Devirtualize TokenMap #47274

Conversation

benaadams commented Aug 31, 2020 • edited Loading

333fred Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

benaadams Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

333fred Aug 31, 2020

Choose a reason for hiding this comment

333fred Aug 31, 2020

Choose a reason for hiding this comment

benaadams Aug 31, 2020

Choose a reason for hiding this comment

333fred Aug 31, 2020

Choose a reason for hiding this comment

jaredpar Aug 31, 2020

Choose a reason for hiding this comment

333fred commented Aug 31, 2020

jaredpar Aug 31, 2020

Choose a reason for hiding this comment

benaadams Aug 31, 2020

Choose a reason for hiding this comment

benaadams commented Sep 5, 2020

ghost commented Sep 5, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost left a comment

Choose a reason for hiding this comment

333fred commented Sep 5, 2020

benaadams commented Aug 31, 2020 •

edited

Loading

333fred Aug 31, 2020 •

edited

Loading

benaadams Aug 31, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.