Cleanup/perf in creating member maps. #67997

CyrusNajmabadi · 2023-04-27T15:13:17Z

CyrusNajmabadi · 2023-04-27T15:14:36Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

-                if (builder != null)
+            static ImmutableArray<TNamespaceOrTypeSymbol> createMembers(object value)
+            {
+                if (value is ArrayBuilder<TNamespaceOrTypeSymbol> builder)


no need for 'hasNamespaces' flag and complex data flow. Once we see a namespace, we know we have to return the builder directly as an ImmutableArray of namespaces or types.

CyrusNajmabadi · 2023-04-27T15:15:00Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

-                        : ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutable<TNamedTypeSymbol>());
-
-                    builder.Free();
+                    return ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutableAndFree<TNamedTypeSymbol>());


if we get to end of the loop, we know we didn't have any namespaes, so no need to check any variablie and can just return teh downcasted values.

CyrusNajmabadi · 2023-04-27T15:15:55Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs


-                var builder = value as ArrayBuilder<TNamespaceOrTypeSymbol>;
-                if (builder != null)
+            static ImmutableArray<TNamespaceOrTypeSymbol> createMembers(object value)


moving into local function eliminates complex control flow and tracking of variables. high level algorithm just walks the dictionary, transforming directly into a new dictionary.

CyrusNajmabadi · 2023-04-27T15:17:37Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

-            {
-                ImmutableArray<TNamespaceOrTypeSymbol> members = kvp.Value;
+            foreach (var (name, members) in map)
+                dictionary.Add(name, getOrCreateNamedTypes(members));


similarly, top level algorithm is just a walk on the dictionary, producing a new dictionary by transforming the values. I was considering a helper that both methods could call, but i was ok just inlining the foreaches in each. lmk if you'd prefer unification given the closeknit nature of these two methods.

CyrusNajmabadi · 2023-04-27T15:18:23Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs


-                bool hasType = false;
-                bool hasNamespace = false;


no need for any tracking flags, or having to deal with any of the combinations of false/true, true/false, true/true (false/false can't happen as the value is always present and has at least one element it represents)

CyrusNajmabadi · 2023-04-27T15:19:31Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

+                // See if creator 'map' put a downcasted ImmutableArray<TNamedTypeSymbol> in it.  If so, we can just directly
+                // downcast to that and trivially reuse it.  If not, that means the array must have contained at least one
+                // TNamespaceSymbol and we'll need to filter that out.
+                var membersAsNamedTypes = members.As<TNamedTypeSymbol>();


we know that if we only had named types then CreateNameToMembersMap already put in a downcasted IA, so we can just directly try to move to that. no need for walking the arrays again.

CyrusNajmabadi · 2023-04-27T17:53:47Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

                    }

-                    members = hasNamespaces
-                        ? builder.ToImmutable()


ToImmutable is strictly worse than ToImmutableAndFree. The latter knows hte builder is being freed, so it can directly move the internal array into the final ImmutableArray (if the counts match). The former always has to copy for teh non-zero element case, which is unnecessary when a move is possible.

CyrusNajmabadi · 2023-04-27T17:56:42Z

src/Dependencies/PooledObjects/ArrayBuilder.cs

+            var result = ToDowncastedImmutable<U>();
+            this.Free();
+            return result;
+        }


i don't feel strongly about having this extension. would be ok inlining it. But it fits the pattern of how we use ArrayBuilders for the other ops, so i kept this in sync with those.

CyrusNajmabadi · 2023-04-27T17:57:23Z

This is ready for review.

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

AlekseyTs · 2023-04-28T13:37:06Z

         , TNamespaceSymbol

It looks like this type parameter is no longer used in any meaningful way.

In reply to: 1527577621

Refers to: src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs:947 in f51fea2. [](commit_id = f51fea2, deletion_comment = False)

AlekseyTs · 2023-04-28T13:38:39Z

Done with review pass (commit 2)

AlekseyTs · 2023-04-28T18:30:21Z

         , TNamespaceSymbol

It looks like this type parameter is no longer used in any meaningful way.

There was no response to this comment

In reply to: 1527577621

Refers to: src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs:947 in f51fea2. [](commit_id = f51fea2, deletion_comment = False)

CyrusNajmabadi · 2023-04-28T19:54:47Z

There was no response to this comment

Odd. These comments don't appear to be in the source code. Something seems weird there. I was able to map this back though to the code you were asking about.

AlekseyTs

LGTM (commit 9)

CyrusNajmabadi · 2023-05-01T03:30:29Z

@RikkiGibson ptal :)

cston · 2023-05-01T23:26:48Z

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs

-                        : ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutable<TNamedTypeSymbol>());
-
-                    builder.Free();
+                    return ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutableAndFree<TNamedTypeSymbol>());


ImmutableArray.CastUp(builder.ToDowncastedImmutableAndFree())

Is this different from builder.ToImmutableAndFree()?

answered offline.

* upstream/main: (2060 commits) implement code folding for anonymous objects Cleanup/perf in creating member maps. (dotnet#67997) Address feedback, clean the use of ILegacyGlobalOptionsWorkspaceService Update Publish.json Semantic snippets: Add inline statement snippets (dotnet#67819) Update VSSDK Build tools version Update doc comment Address feedback Move declaration near reference Address feedback Avoid stack overflow due to deep recursion on long chain of calls. (dotnet#67913) Check ILegacyGlobalOptionsWorkspaceService is null test scout queue add test update editor "Where" clause typo fix (dotnet#68002) Use `Keyword` helper method instead of hardcoding keyword help terms Move the SyntaxTree and SemanticModel action based analyzers to respect context.FilterSpan EnC refactoring: Align wrappers with contracts better (dotnet#67967) Include EA.RazorCompiler in source build (dotnet#67996) ...

Cleanup/perf in creating member maps.

5e19a7f

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 27, 2023

CyrusNajmabadi commented Apr 27, 2023

View reviewed changes

Simplify

f51fea2

CyrusNajmabadi commented Apr 27, 2023

View reviewed changes

build-analysis bot mentioned this pull request Apr 27, 2023

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

5 tasks

CyrusNajmabadi marked this pull request as ready for review April 27, 2023 17:51

CyrusNajmabadi requested a review from a team as a code owner April 27, 2023 17:51

CyrusNajmabadi changed the title ~~WIP: Not ready for review.~~ Cleanup/perf in creating member maps. Apr 27, 2023

CyrusNajmabadi commented Apr 27, 2023

View reviewed changes

AlekseyTs reviewed Apr 28, 2023

View reviewed changes

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs Show resolved Hide resolved

AlekseyTs reviewed Apr 28, 2023

View reviewed changes

src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits April 28, 2023 09:24

Merge remote-tracking branch 'upstream/main' into membersCleanup

f110c34

Better assert, and avoid empty arrays

05fa757

CyrusNajmabadi added 5 commits April 28, 2023 12:35

Remove type argument

b11a6c6

Remove unnecessary type arguments

7d644dc

Fix

876da53

Make static

e6b5c2d

Add assert

8f67daa

AlekseyTs approved these changes Apr 28, 2023

View reviewed changes

cston reviewed May 1, 2023

View reviewed changes

cston approved these changes May 1, 2023

View reviewed changes

CyrusNajmabadi merged commit 1a24cf4 into dotnet:main May 1, 2023

CyrusNajmabadi deleted the membersCleanup branch May 1, 2023 23:39

ghost added this to the Next milestone May 1, 2023

allisonchou mentioned this pull request May 4, 2023

[Automated] PRs inserted in VS build main-33703.465 #68085

Closed

Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup/perf in creating member maps. #67997

Cleanup/perf in creating member maps. #67997

CyrusNajmabadi commented Apr 27, 2023 •

edited

Loading

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi Apr 27, 2023

CyrusNajmabadi commented Apr 27, 2023

AlekseyTs commented Apr 28, 2023 •

edited

Loading

AlekseyTs commented Apr 28, 2023

AlekseyTs commented Apr 28, 2023

CyrusNajmabadi commented Apr 28, 2023

AlekseyTs left a comment

CyrusNajmabadi commented May 1, 2023

cston May 1, 2023

CyrusNajmabadi May 1, 2023

Cleanup/perf in creating member maps. #67997

Cleanup/perf in creating member maps. #67997

Conversation

CyrusNajmabadi commented Apr 27, 2023 • 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

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 27, 2023

AlekseyTs commented Apr 28, 2023 • edited Loading

AlekseyTs commented Apr 28, 2023

AlekseyTs commented Apr 28, 2023

CyrusNajmabadi commented Apr 28, 2023

AlekseyTs left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 27, 2023 •

edited

Loading

AlekseyTs commented Apr 28, 2023 •

edited

Loading