Collection expressions: use inline array for ReadOnlySpan<T> argument to builder method by cston · Pull Request #69227 · dotnet/roslyn

cston · 2023-07-26T07:18:40Z

Use inline array type instances as the storage for spans passed to collection expression builder methods. The inline array types are synthesized by the compiler and emitted as internal types in the assembly.

Relates to test plan #66418

… to builder method

CyrusNajmabadi · 2023-07-27T22:54:39Z

Will review tomorrow!

jcouv · 2023-07-27T22:57:41Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+        internal override FileIdentifier? AssociatedFileIdentifier => null;
+
+        internal override bool MangleName => false; // PROTOTYPE: Is this correct?


// PROTOTYPE: Is this correct?

PROTOTYPE comments are not allowed in main branch #Closed

jcouv · 2023-07-27T22:58:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            var sideEffects = ArrayBuilder<BoundExpression>.GetInstance();
+            BoundExpression span;
+
+            // PROTOTYPE: Check also ReadOnlySpan<T> parameter is not [UnscopedRef].


PROTOTYPE comment (also elsewhere) #Closed

jcouv · 2023-07-28T02:12:07Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

+
+            string typeName = PrivateImplementationDetails.GetInlineArrayTypeName(arrayLength);
+            var privateImplClass = GetPrivateImplClass(syntaxNode, diagnostics);
+            var typeAdapter = privateImplClass.GetType(typeName);


GetType

nit: consider calling this TryGetType since it can fail to return a type. Then maybe consider using a TryGet API pattern (return bool, out parameter). #Closed

I went with GetType() to match the existing GetMethod() method used in the other Ensure methods here.

jcouv · 2023-07-28T02:12:24Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

+
+                var typeSymbol = new SynthesizedInlineArrayTypeSymbol(SourceModule, typeName, arrayLength, attributeConstructor);
+                privateImplClass.TryAddSynthesizedType(typeSymbol.GetCciAdapter());
+                typeAdapter = privateImplClass.GetType(typeName)!;


!

Do we need this suppression? #Closed

We're dereferencing typeAdapter below.

nit: consider doing Debug.Assert(typeAdapter!.Name == typeName); instead. That assertion should also ensure the nullable state of typeAdapter is not-null.

jcouv · 2023-07-28T02:15:13Z

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

+        internal static string GetInlineArrayTypeName(int arrayLength)
+        {
+            Debug.Assert(arrayLength > 0);
+            return $"$InlineArray{arrayLength}";


nit: the other nested types we add in private implementation details have a different naming convention. Consider aligning. Here's one that was recently added:

public string Name => _alignment == 1 ? $"__StaticArrayInitTypeSize={_size}" : $"__StaticArrayInitTypeSize={_size}_Align={_alignment}"; ``` #Closed

Changed to __InlineArrayN<T> but perhaps __InlineArrayTypeSize=N<T> might be better, or at least more consistent. Feel free to suggest specific names.

jcouv · 2023-07-28T02:16:26Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.Symbols
+{
+    internal sealed class SynthesizedInlineArrayTypeSymbol : NamedTypeSymbol


SynthesizedInlineArrayTypeSymbol

nit: doc? #Closed

jcouv · 2023-07-28T02:25:21Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+        public override bool IsReadOnly => true;
+
+        public override Symbol? ContainingSymbol => null;


Why is ContainingSymbol null? #Closed

jcouv · 2023-07-28T02:27:17Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+        internal override ObsoleteAttributeData? ObsoleteAttributeData => null;
+
+        public override ImmutableArray<Symbol> GetMembers() => ImmutableArray<Symbol>.Empty;


Shouldn't we return the field here? #Closed

jcouv · 2023-07-28T02:27:42Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+        public override ImmutableArray<Symbol> GetMembers() => ImmutableArray<Symbol>.Empty;
+
+        public override ImmutableArray<Symbol> GetMembers(string name) => GetMembers().WhereAsArray(m => m.Name == name);


Could be specialized to just return the name of the field #Closed

The current implementation seems more robust, and WhereAsArray() should avoid any allocations since the result should be either the entire array or an empty array.

jcouv · 2023-07-28T02:41:40Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+            public override ImmutableArray<Location> Locations => ImmutableArray<Location>.Empty;
+
+            public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty;


nit: Why return a result here, but throw on SynthesizedInlineArrayTypeSymbol.DeclaringSyntaxReferences? #Closed

jcouv · 2023-07-28T02:47:12Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            BoundExpression span;
+
+            // PROTOTYPE: Check also ReadOnlySpan<T> parameter is not [UnscopedRef].
+            if (elements.Length > 0


Should there be any kind of upper limit/heuristic? #Closed

For now, we don't think there should be a limit (see notes), because the elements are already explicit in the source and because the work around to use the heap is to use C# 11 syntax.

jcouv · 2023-07-28T02:49:40Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            Debug.Assert(inlineArrayType.HasInlineArrayAttribute(out int inlineArrayLength) && inlineArrayLength == arrayLength);
+
+            var intType = _factory.SpecialType(SpecialType.System_Int32);
+            var elementRef = _factory.ModuleBuilderOpt.EnsureInlineArrayElementRefExists(syntax, intType, _diagnostics.DiagnosticBag);


elementRef

What's the type of this? #Closed

jcouv · 2023-07-28T02:51:05Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+
+            var intType = _factory.SpecialType(SpecialType.System_Int32);
+            var elementRef = _factory.ModuleBuilderOpt.EnsureInlineArrayElementRefExists(syntax, intType, _diagnostics.DiagnosticBag);
+            elementRef = elementRef.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));


elementRef =

nit: consider doing a chain call to .Construct as couple of lines above #Closed

jcouv · 2023-07-28T02:53:25Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            for (int i = 0; i < arrayLength; i++)
+            {
+                var element = VisitExpression(elements[i]);
+                var call = _factory.Call(null, elementRef, inlineArrayLocal, _factory.Literal(i), useStrictArgumentRefKinds: true);


Please add pseudo-code comments corresponding to what is generated #Closed

Thanks for adding. It makes things much easier to follow :-)

jcouv · 2023-07-28T02:58:12Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            var inlineArrayAsSpan = _factory.ModuleBuilderOpt.EnsureInlineArrayAsSpanExists(syntax, spanType, intType, _diagnostics.DiagnosticBag);
+            inlineArrayAsSpan = inlineArrayAsSpan.Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));
+
+            spanType = spanType.Construct(ImmutableArray.Create(elementType));


This was already constructed by EnsureInlineArrayAsSpanExists/SynthesizedInlineArrayAsSpanMethod. Consider grabbing the return type from there. #Closed

cston · 2023-07-31T06:41:12Z

    public void CollectionBuilder_MissingSpanMembers(bool useCompilationReference)

There is nothing to test here after switching to InlineArrayAsReadOnlySpan() since the Span<T> members are no longer referenced. (And if we test missing ReadOnlySpan<T> members instead, then binding will report an error that the collection cannot be constructed because the builder method uses ReadOnlySpan<T> as the parameter type.)

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:9263 in 4cfe4e1. [](commit_id = 4cfe4e1, deletion_comment = True)

jcouv · 2023-07-31T18:57:26Z

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

+        private readonly ConcurrentDictionary<string, Cci.INamedTypeDefinition> _synthesizedInlineArrayTypes =
+            new ConcurrentDictionary<string, Cci.INamedTypeDefinition>();
+
        // field types for different block sizes.


This comment seems out-of-date #Closed

jcouv · 2023-07-31T19:03:32Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+                Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));
+
+            // Create an inline array and assign to a local.
+            // var tmp = new __InlineArrayN<T>();


T

nit: ElementType instead of T (also below)? #Closed

jcouv · 2023-07-31T19:05:29Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedInlineArrayTypeSymbol.cs

+
+        public override bool IsReadOnly => true;
+
+        public override Symbol? ContainingSymbol => _containingModule.GlobalNamespace;


Is this the right containing symbol? I thought this is a nested type inside the private implementation type (based on _orderedNestedTypes = orderedProxyTypes.Concat(orderedInlineArrayTypes)) #Closed

The types are actually top-level types even though they are visited in emit through PrivateImplementationDetails.GetNestedTypes().

Let's test that they can be referenced in any way in source (when loading from metadata), not merely inaccessible.

From offline discussion, let's leave a comment here if the symbol cannot reference the proper containing symbol. Let's try to emit those types are nested types in the private implementation.

Changed inline array types to be top-level types with names that cannot be used in source.

jcouv · 2023-07-31T19:13:55Z

        var verifier = CompileAndVerify(new[] { source, s_collectionExtensionsWithSpan }, targetFramework: TargetFramework.Net70, verify: Verification.Skipped, expectedOutput: IncludeExpectedOutput("[], [1, 2], [3, 4, 5], [null, 7], "));

nit: consider breaking line (also applies below)

In reply to: 1658987553

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:2691 in 75ead79. [](commit_id = 75ead79, deletion_comment = False)

jcouv · 2023-07-31T19:37:24Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            if (elements.Length > 0
+                && !elements.Any(i => i is BoundCollectionExpressionSpreadElement)
+                && _compilation.Assembly.RuntimeSupportsInlineArrayTypes
+                && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue))


Did we add tests to cover those conditions, or were those getting hit by existing scenarios? #Closed

Covered by CollectionBuilder_RefStructCollection.

jcouv · 2023-07-31T19:38:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

+            var inlineArrayType = _factory.ModuleBuilderOpt.EnsureInlineArrayTypeExists(syntax, _factory, arrayLength, _diagnostics.DiagnosticBag).Construct(ImmutableArray.Create(elementType));
+            Debug.Assert(inlineArrayType.HasInlineArrayAttribute(out int inlineArrayLength) && inlineArrayLength == arrayLength);
+
+            var intType = _factory.SpecialType(SpecialType.System_Int32);


Do we have a test where type System.Int32 is missing?

No. I believe we're handling that correctly here, and I don't think we'd get this far without an error in that case.

We have tests for various other missing types, but not this one. Let's cover it too.

jcouv · 2023-07-31T19:41:02Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

+            var privateImplClass = GetPrivateImplClass(syntaxNode, diagnostics);
+            var typeAdapter = privateImplClass.GetType(typeName);
+
+            if (typeAdapter is null)


Do we have a test with two collection expressions of the same size? (the InlineArray type will be re-used/shared) #Closed

CollectionBuilder_InlineArrayTypes

jcouv

Done with review (iteration 7)

jcouv · 2023-08-01T16:19:16Z

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

            return method;
        }

+        internal Cci.INamespaceTypeDefinition? GetType(string name)


GetType

nit: naming this GetSynthesizedInlineArrayType would make more sense. GetType is confusing/misleading. Same comment for TryAddSynthesizedType. Alternatively, we can rename _synthesizedInlineArrayTypes to something less specific like _additionalTopLevelTypes. #Closed

jcouv · 2023-08-01T16:22:25Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

+            Debug.Assert(Compilation.Assembly.RuntimeSupportsInlineArrayTypes);
+            Debug.Assert(arrayLength > 0);
+
+            string typeName = $"<>y__InlineArray{arrayLength}"; // see GeneratedNameKind.InlineArrayType


GeneratedNameKind.InlineArrayType

Consider $"<>{GeneratedNameKind.InlineArrayType}__InlineArray{arrayLength}" or adding an assertion Debug.Assert("y" == GeneratedNameKind.InlineArrayType)

That way, GoToDefinition on GeneratedNameKind.InlineArrayType leads somewhere useful and that we don't accidentally modify it.
#Closed

jcouv

Done with review pass (iteration 12)

jcouv

LGTM Thanks (iteration 13)

jcouv

LGTM Thanks (iteration 14)

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 26, 2023

cston force-pushed the collections-ia-arg branch 4 times, most recently from f975cc1 to 4176dcd Compare July 27, 2023 21:56

Collection expressions: use inline array for ReadOnlySpan<T> argument…

d75c020

… to builder method

cston force-pushed the collections-ia-arg branch from 4176dcd to d75c020 Compare July 27, 2023 22:08

cston marked this pull request as ready for review July 27, 2023 22:12

cston requested a review from a team as a code owner July 27, 2023 22:12

cston requested review from 333fred, CyrusNajmabadi and RikkiGibson July 27, 2023 22:12

jcouv reviewed Jul 27, 2023

View reviewed changes

RikkiGibson self-assigned this Jul 27, 2023

jcouv reviewed Jul 28, 2023

View reviewed changes

cston force-pushed the collections-ia-arg branch from 788b882 to 4cfe4e1 Compare July 31, 2023 06:22

Remove unneeded WellKnownMember

75ead79

jcouv reviewed Jul 31, 2023

View reviewed changes

Misc.

d36040b

jcouv reviewed Jul 31, 2023

View reviewed changes

cston added 4 commits July 31, 2023 12:54

Address feedback

99717d6

Make inline array type a top-level type

d4c2081

Break lines

c419b51

Fix build

60e248e

jcouv reviewed Aug 1, 2023

View reviewed changes

Address feedback

adae5cd

jcouv approved these changes Aug 1, 2023

View reviewed changes

Add back deleted test

6fc3312

jcouv approved these changes Aug 1, 2023

View reviewed changes

cston merged commit 7183a8a into dotnet:main Aug 1, 2023

cston deleted the collections-ia-arg branch August 1, 2023 20:58

ghost added this to the Next milestone Aug 1, 2023

cston mentioned this pull request Aug 1, 2023

Implement optimizations for constructing collections from collection literals #68785

Open

17 tasks

allisonchou mentioned this pull request Aug 3, 2023

[Automated] PRs inserted in VS build main-34003.103 #69363

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023


		internal override FileIdentifier? AssociatedFileIdentifier => null;

		internal override bool MangleName => false; // PROTOTYPE: Is this correct?


		public override bool IsReadOnly => true;

		public override Symbol? ContainingSymbol => null;


		internal override ObsoleteAttributeData? ObsoleteAttributeData => null;

		public override ImmutableArray<Symbol> GetMembers() => ImmutableArray<Symbol>.Empty;


		public override ImmutableArray<Location> Locations => ImmutableArray<Location>.Empty;

		public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => ImmutableArray<SyntaxReference>.Empty;

Conversation

cston commented Jul 26, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jul 27, 2023 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 27, 2023 • 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!

jcouv Jul 27, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

cston Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • 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!

jcouv Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cston commented Jul 26, 2023 •

edited by jcouv

Loading

CyrusNajmabadi commented Jul 27, 2023 •

edited by jcouv

Loading

jcouv Jul 27, 2023 •

edited

Loading

jcouv Jul 27, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

cston Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 28, 2023 •

edited

Loading

jcouv Jul 31, 2023 •

edited

Loading

jcouv Jul 31, 2023 •

edited

Loading

jcouv Jul 31, 2023 •

edited

Loading

jcouv commented Jul 31, 2023 •

edited by cston

Loading

jcouv Jul 31, 2023 •

edited

Loading

jcouv Jul 31, 2023 •

edited

Loading