Reduce allocations in PENamespaceSymbol.GetMembers()

[ToddGrun]

This method commonly shows up in profiles, and this change is very similar to an optimization made to make GetTypeMembers more efficient.

Just opening roslyn.sln and bringing up completion once in a file ends up hitting this codepath about 5,000 times in VS and about 50,000 times OOP. With these changes, I see about 60% of those requests in VS hit the cache, and about 95% of those OOP hit the cache. This is about worst case for hits in this cache too, as PENamespaceSymbol object seemed to be reused quite well. 

[AlekseyTs]

    public sealed override ImmutableArray<Symbol> GetMembers()

My understanding of the goal of this change is to cache and reuse result produced by this method. If the understanding is correct, then there is an unnecessary churn in this change. I would prefer the change to have the following form:

1. The portion of the body past EnsureAllMembersLoaded(); is extracted into a helper local function (no changes to the algorithm or types).
2. The result of that local function is cached/reused

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamespaceSymbol.cs:67 in 79eb566. [](commit_id = 79eb566, deletion_comment = False)

[AlekseyTs]

Done with review pass (commit 2)

[AlekseyTs]

Can VB implementation benefit from a similar change?

[ToddGrun]

    public sealed override ImmutableArray<Symbol> GetMembers()

I've made changes in commit 4 such that I believe the code meets these requests.

---

In reply to: 2142788430

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamespaceSymbol.cs:67 in 79eb566. [](commit_id = 79eb566, deletion_comment = False)

[ToddGrun]

The GetMembers code in VB appears to already have a single data structure which it can use

---

In reply to: 2142864567

[ToddGrun]

@RikkiGibson -- any remaining concerns?

[AlekseyTs]

The GetMembers code in VB appears to already have a single data structure which it can use

I am not sure what this means and how it relates to the goal of this PR. It looks to me that result of PENamespaceSymbol.GetMembers in VB is not cached/reused.

---

In reply to: 2143487490

[AlekseyTs]

    public sealed override ImmutableArray<Symbol> GetMembers()

I've made changes in commit 4 such that I believe the code meets these requests.

It doesn't look like this to me. I was asking to extract the original code without any changes that, in my opinion, are completely unnecessary to achieve the goal of this PR. Then to add cache/reuse logic for the result, but placing the logic outside of the local function.

This is what I have in mind.

        /// <summary>
        /// All namespace and type members in a flat array
        /// </summary>
        private ImmutableArray<Symbol> _lazyFlattenedNamespacesAndTypes;

        ...

        [PerformanceSensitive(
            "[https://github.com/dotnet/roslyn/issues/23582](https://github.com/dotnet/roslyn/issues/23582)",
            Constraint = "Provide " + nameof(ArrayBuilder<Symbol>) + " capacity to reduce number of allocations.",
            AllowGenericEnumeration = false)]
        public sealed override ImmutableArray<Symbol> GetMembers()
        {
            if (_lazyFlattenedNamespacesAndTypes.IsDefault)
            {
                EnsureAllMembersLoaded();
                ImmutableInterlocked.InterlockedExchange(ref _lazyFlattenedNamespacesAndTypes, calculateMembers());
            }

            return _lazyFlattenedNamespacesAndTypes;

            ImmutableArray<Symbol> calculateMembers()
            {
                var memberTypes = GetMemberTypesPrivate();

                if (lazyNamespaces.Count == 0)
                    return StaticCast<Symbol>.From(memberTypes);

                var builder = ArrayBuilder<Symbol>.GetInstance(memberTypes.Length + lazyNamespaces.Count);

                builder.AddRange(memberTypes);
                foreach (var pair in lazyNamespaces)
                {
                    builder.Add(pair.Value);
                }

                return builder.ToImmutableAndFree();
            }
        }

---

In reply to: 2143487169

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamespaceSymbol.cs:67 in 79eb566. [](commit_id = 79eb566, deletion_comment = False)

[AlekseyTs]

Done with review pass (commit 3)

[ToddGrun]

    public sealed override ImmutableArray<Symbol> GetMembers()

I do like that better. Changing lazyFlattenedNamespacesAndTypes to ImmutableArray made things a bit easier.

---

In reply to: 2146526960

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamespaceSymbol.cs:67 in 79eb566. [](commit_id = 79eb566, deletion_comment = False)

[ToddGrun]

I am not sure what this means and how it relates to the goal of this PR. It looks to me that result of PENamespaceSymbol.GetMembers in VB is not cached/reused.

Yeah, I think we're having a mismatch here. I'm looking at this line:

roslyn/src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/PENamespaceSymbol.vb

Line 73 in 824df41

Public NotOverridable Overloads Overrides Function GetMembers() As ImmutableArray(Of Symbol)

My understanding is that this is the VB equivalent of the code I am modifying in this PR. That code just returns a flattened version of m_lazyMembers, and doesn't combine that with any other data structure. I'd prefer not to change that unless you feel it's necessary.

[AlekseyTs]

That code just returns a flattened version of m_lazyMembers, and doesn't combine that with any other data structure.

What does Flatten do?

[ToddGrun]

What does Flatten do?

Looks like it takes takes a Dictionary whose values are ImmutableArrays, and flattens those immutable arrays into a single array.

[AlekseyTs]

What does Flatten do?

Looks like it takes takes a Dictionary whose values are ImmutableArrays, and flattens those immutable arrays into a single array.

Is this going to allocate a new ImmutableArray every time? If so, isn't that what we were trying to avoid for C#? The PR title says "
Reduce allocations", isn't this our goal?

[ToddGrun]

isn't this our goal?

It certainly is!

However, I haven't yet seen a trace implicating this code in VB, whereas I've seen the C# GetMembers call show up in dozens of traces I've looked at. I can indeed take a look at optimizing the VB scenario too, but since it differed from the C# code in this area, I didn't think it within the scope of what I was trying to achieve.

[AlekseyTs]

However, I haven't yet seen a trace implicating this code in VB, whereas I've seen the C# GetMembers call show up in dozens of traces I've looked at.

I can certainly buy this argument

but since it differed from the C# code in this area

This argument, however, I do not understand.

[AlekseyTs]

Done with review pass (commit 5).

[AlekseyTs]

LGTM (commit 6)

[AlekseyTs]

@RikkiGibson, @dotnet/roslyn-compiler For the second review.