Implement ImmutableSegmentedDictionary<TKey, TValue> #50189

sharwell · 2020-12-31T06:11:01Z

Provides an implementation of IImmutableDictionary<TKey, TValue> which is backed by SegmentedDictionary<TKey, TValue>. The immutable wrapper is a value type, offering benefits similar to the cases where ImmutableArray<T> is more appropriate than ImmutableList<T>. The implementation preserves all functionality from ImmutableDictionary<TKey, TValue> (intended to be a near drop-in replacement), with the following exceptions:

The value comparer of ImmutableDictionary<TKey, TValue> is customizable through the ValueComparer property, but this collection always uses EqualityComparer<TValue>.Default
ImmutableDictionary<TKey, TValue> does not provide struct enumerators for the Keys or Values property (Memory allocation documentation for ImmutableDictionary Keys and Values runtime#13927), but ImmutableSegmentedDictionary<TKey, TValue> does

This comparer is not required by IImmutableDictionary<TKey, TValue> and is not directly supported by SegmentedDictionary<TKey, TValue>. Removing the property and relying on EqualityComparer<TValue>.Default allows for more efficient wrapper implementation.

sharwell · 2021-01-20T15:31:46Z

@333fred @tmat @jaredpar for reviews

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+Builder.cs

tmat · 2021-01-20T17:42:27Z

src/Dependencies/Collections/ImmutableSegmentedDictionary.cs

+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;


Need to add #nullable enable so that it gets enabled in projects that don't have it enabled by default.

➡️ The current code follows the patterns established in the Microsoft.CodeAnalysis.PooledObjects.Package package. If we want to change the way we annotate source packages, we can do so but it should be a separate pull request that includes validation to prevent subtle regressions.

Sounds good. Let's follow up.

➡️ #50749

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs

src/Dependencies/Collections/RoslynImmutableInterlocked.cs

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+PrivateInterlocked.cs

333fred · 2021-01-20T18:07:20Z

Done review pass (commit 4)

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+PrivateInterlocked.cs

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs

tmat · 2021-01-20T18:25:07Z

src/Dependencies/Collections/RoslynImmutableInterlocked.cs

+
+namespace Microsoft.CodeAnalysis.Collections
+{
+ internal static class RoslynImmutableInterlocked


RoslynImmutableInterlocke [](start = 26, length = 25)

I'd move these helpers to ImmutableSegmentedDictionary static class. Exposing "RoslynImmutableInterlocked" type is odd.

➡️ This type is intentionally designed as a drop-in replacement for code currently using ImmutableInterlocked with ImmutableDictionary<TKey, TValue>.

That may be, but I don't like these type names. You can drop-in replace as well, just use different type names.

These names follow the pattern of RoslynDebug and RoslynString. The methods it contains are exactly the same as the methods in ImmutableInterlocked (signatures and semantics), but with signatures adjusted to work with our types in ways that ImmutableInterlocked does not support. I can rename this to ImmutableInterlocked as soon as C# is updated to allow static extensions to existing static classes.

The name can't otherwise be changed without actively interfering with a-priori developer knowledge.

RoslynDebug and RoslynString are internal types limited to Roslyn repo and not exposed by a source package. What if we had another type that needs interlocked operations? Are we gonna have RoslynImmutableInterlocked2?

ImmutableInterlocked provides atomic operations for all of the immutable collections types.

That would require either static extension methods or all immutable types defined in the same assembly.

Other members are going to be added to this type when ImmutableSegmentedList is added

SegmentedCollectionsInterlocked?

That works for ImmutableSegmentedDictionary<TKey, TValue> and ImmutableSegmentedList<T>, but not for ImmutableArrayDictionary<TKey, TValue>.

I'll file a follow-up issue to discuss naming for static helper types in source packages.

Is the plan to move these to System.Collections.Immutable at some point?

Filed #50750. Moving to System.Collections.Immutable is an option, though I'm not sure if/when that would occur.

src/Dependencies/Collections/RoslynImmutableInterlocked.cs

These files are based on dotnet/runtime@cb5f173b (v5.0.2).

src/Compilers/Core/CodeAnalysisTest/Collections/DebuggerAttributes.cs

333fred · 2021-01-22T19:27:13Z

src/Compilers/Core/CodeAnalysisTest/Collections/ImmutableSegmentedDictionaryBuilderTest.cs


 mutable.Add(1, "a");
 var immutable2 = mutable.ToImmutable();
- Assert.NotSame(immutable1, immutable2); // "Mutating the collection did not reset the Immutable property."
- Assert.Same(immutable2, mutable.ToImmutable()); // "The Immutable property getter is creating new objects without any differences."
+ Assert.False(immutable1 == immutable2); // "Mutating the collection did not reset the Immutable property."


Consider using Equal/NotEqual for these, rather than True/False with operators, as that will give better errors.

This test is leveraging the fact that == implements reference equality for these collections (similar to ImmutableArray<T>). While Equals technically would do the same thing for this type, it feels one step further removed because it looks like value equality is intended by the test when we're really looking for reference equality.

I don't believe that outweighs the benefits from getting clear error messages.

➡️ Switched to Assert.False(IsSame(...)) to match the other cases

I don't believe that addresses my comments about error messages.

This is correct, but also By Design in accordance with our adopted test framework (see xunit/xunit#350 for additional discussion). The approach used by this pull request adheres to the test framework recommendation that the assertion call site clearly show the intent of the assertion, which IsSame will do.

333fred

I'm not going to block the PR over the test comments, but I really would prefer if our tests had good error messages for failures.

Dotnet-GitSync-Bot added the Area-IDE label Dec 31, 2020

tmat mentioned this pull request Jan 12, 2021

Document state storage #50298

Closed

sharwell added 4 commits January 12, 2021 16:20

Implement ImmutableSegmentedDictionary<TKey, TValue>

a127f40

Convert ImmutableSegmentedDictionary<TKey, TValue> to a value type

a06d24e

Add RoslynImmutableInterlocked to provide atomic operations

8ecf29e

sharwell force-pushed the immutable-segmented-dictionary branch from 8fa13a5 to 8ecf29e Compare January 13, 2021 00:20

sharwell marked this pull request as ready for review January 13, 2021 00:20

tmat added the Area-Compilers label Jan 20, 2021

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+Builder.cs Outdated Show resolved Hide resolved

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+Builder.cs Outdated Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs Outdated Show resolved Hide resolved

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/RoslynImmutableInterlocked.cs Outdated Show resolved Hide resolved

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/RoslynImmutableInterlocked.cs Show resolved Hide resolved

333fred reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/RoslynImmutableInterlocked.cs Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+PrivateInterlocked.cs Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2+PrivateInterlocked.cs Outdated Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/ImmutableSegmentedDictionary`2.cs Show resolved Hide resolved

tmat reviewed Jan 20, 2021

View reviewed changes

src/Dependencies/Collections/RoslynImmutableInterlocked.cs Outdated Show resolved Hide resolved

sharwell mentioned this pull request Jan 20, 2021

Implement debugger attributes for ImmutableSegmentedDictionary<TKey, TValue> #50657

Open

sharwell added 4 commits January 20, 2021 16:00

Fix warning suppressions

fc540b9

Pass by readonly reference to VolatileRead

dd02a7d

Add ImmutableDictionary<TKey, TValue> tests

1b0e27b

These files are based on dotnet/runtime@cb5f173b (v5.0.2).

Add origin notes

cdca847

sharwell added 5 commits January 20, 2021 16:00

Update code for use in Roslyn

b7eef41

Rename types in preparation for testing ImmutableSegmentedDictionary

0955756

Enable tests for ImmutableSegmentedDictionary

dde2b1d

Simplify code flow in RoslynImmutableInterlocked

16d87f7

Add more comments

54600cd

sharwell force-pushed the immutable-segmented-dictionary branch from 2ffe81e to 54600cd Compare January 21, 2021 00:11

sharwell requested a review from a team as a code owner January 21, 2021 00:11

333fred reviewed Jan 22, 2021

View reviewed changes

src/Compilers/Core/CodeAnalysisTest/Collections/DebuggerAttributes.cs Show resolved Hide resolved

333fred reviewed Jan 22, 2021

View reviewed changes

Use 'IsSame' consistently in tests

a5b0f5b

This was referenced Jan 25, 2021

Consider explicit '#nullable' directives in source packages #50749

Open

Source packages expose internal 'Roslyn' prefix naming convention #50750

Open

This was referenced Jan 25, 2021

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

333fred approved these changes Jan 25, 2021

View reviewed changes

sharwell merged commit 1857667 into dotnet:master Jan 27, 2021

ghost added this to the Next milestone Jan 27, 2021

sharwell deleted the immutable-segmented-dictionary branch January 27, 2021 08:19

RikkiGibson modified the milestones: Next, 16.10.P1 Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ImmutableSegmentedDictionary<TKey, TValue> #50189

Implement ImmutableSegmentedDictionary<TKey, TValue> #50189

sharwell commented Dec 31, 2020 •

edited

Loading

sharwell commented Jan 20, 2021

tmat Jan 20, 2021

sharwell Jan 21, 2021

tmat Jan 21, 2021 •

edited

Loading

sharwell Jan 25, 2021

333fred commented Jan 20, 2021

tmat Jan 20, 2021

sharwell Jan 20, 2021

tmat Jan 20, 2021

sharwell Jan 21, 2021 •

edited

Loading

tmat Jan 21, 2021

tmat Jan 21, 2021

sharwell Jan 21, 2021 •

edited

Loading

sharwell Jan 25, 2021

tmat Jan 25, 2021

sharwell Jan 25, 2021

333fred Jan 22, 2021

sharwell Jan 22, 2021

333fred Jan 22, 2021

sharwell Jan 23, 2021 •

edited

Loading

333fred Jan 25, 2021

sharwell Jan 25, 2021

333fred left a comment

Implement ImmutableSegmentedDictionary<TKey, TValue> #50189

Implement ImmutableSegmentedDictionary<TKey, TValue> #50189

Conversation

sharwell commented Dec 31, 2020 • edited Loading

sharwell commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

sharwell commented Dec 31, 2020 •

edited

Loading

tmat Jan 21, 2021 •

edited

Loading

sharwell Jan 21, 2021 •

edited

Loading

sharwell Jan 21, 2021 •

edited

Loading

sharwell Jan 23, 2021 •

edited

Loading