-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allocate less in GetTypeByMetadataName #68415
Conversation
{ | ||
var ns = symbol as NamespaceSymbol; | ||
|
||
if ((object)ns != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
named types were fully ignored. so this loop is equivalent to just asking for the nested namespace with the particular name. there can be only one as long as hte compiler is well behaving (The assert below checked for that). so we can just rewrite this entire loop to GetNestedNamespace below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: if we want to still have the protection against MergedNamespaces hitting this codepath, we could do that by just having an #if DEBUG
block that does the original check.
@@ -68,6 +119,8 @@ internal partial struct MetadataTypeName | |||
/// </summary> | |||
private ImmutableArray<string> _namespaceSegments; | |||
|
|||
private ImmutableArray<ReadOnlyMemory<char>> _namespaceSegmentsMemory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a pity this is still an IA (which will allocate the underlying array). Esp. as most cases will be splitting a string with <4 actual namespace segments. Having some slim array-like type that did not hit the heap for the low-number-of-elements case would be ideal here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that was 'TemporaryArray'. I guess we don't want to use that type as a field?
/// <returns>An ImmutableArray containing all the types that are members of this symbol with the given name. | ||
/// If this symbol has no type members with this name, | ||
/// returns an empty ImmutableArray. Never returns null.</returns> | ||
public abstract override ImmutableArray<NamedTypeSymbol> GetTypeMembers(string name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already abstract in base type.
@AlekseyTs I believe feedback is responded to. I either made the requested changes, or i believe i gave a justification on certain aspects as to why i did not. Please take a look at lmk if you have updated thoughts in these areas, or if you'd prefer i do things differently. |
Done with review pass (commit 56) |
f979388
to
6724004
Compare
src/Features/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs
Outdated
Show resolved
Hide resolved
…ctSpellCheckingHandler.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 61), assuming CI is passing
@dotnet/roslyn-compiler For the second review. |
Thanks! |
Fixes #68343
This PR changes GetTypeByMetadataName to (and by proxy teh 'MetadataTypeName' helper) to operate using
ReadOnlyMemory<char>
spans instead of actual System.String instances. Prior to this work, if someone asks for a type for a given (potentially large) string"N1.N2.N3.T1"
, then that string is broken into two pieces first (for the namespace section, and the type section). This adds allocs equivalent to the the original string as garbage.. Then the namespace section is broken into its constituent parts (causing another allocation of nearly the original string as garbage). Then, if the type name is generic (e.g.List`1
), the non-generic version is allocated as well.Now, we instead never break the full string up. Instead, we produce
System.ReadOnlyMemory<char>
ranges over that string that are then used as we walk the symbol table mapping from teh fully-qualified name to final symbol.This required converting a lot of internal state for namespaces and type symbols to key off of
ReadOnlyMemory<char>
instead of strings. But was all fairly straightforward. The savings is significant. String allocations go from 8.8% of all garbage allocs while running, down to 2.6% (a savings of 6.2%, and a lot lower pressure on the GC):