-
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
Changes from 29 commits
633c3c6
6d739c5
10f5607
1ef9d0e
d41b95d
8b3fddc
e02be95
ea4391b
badbdbb
afcacad
439466b
03801fe
a4c5d39
ea6a516
54f9eaa
cb5c485
cb95a2c
a08f317
0e1b119
6230338
0154b49
864a406
1ba1654
98cbf06
6c03588
116a447
ab9aca9
120d13e
f052974
3ea0c33
66f1544
2ccbc86
3584e8a
ae9f5ec
2e04867
a0c934d
ef6dd44
e70e5b6
897366e
038d130
9e1af08
58e2351
5bb17be
5e0c905
35a2062
0a8daed
c63380d
d5c2a5d
23d1112
ee6617f
7f6c0c6
68c465d
179a4bf
84546bb
64c8fc3
5157ad1
eec5c0d
97fa6be
6724004
333dca6
a3d241a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,7 +476,7 @@ private ImmutableArray<CSharpAttributeData> GetCustomAttributesFilterCompilerAtt | |
} | ||
|
||
internal void OnNewTypeDeclarationsLoaded( | ||
Dictionary<string, ImmutableArray<PENamedTypeSymbol>> typesDict) | ||
Dictionary<ReadOnlyMemory<char>, ImmutableArray<PENamedTypeSymbol>> typesDict) | ||
{ | ||
bool keepLookingForDeclaredCorTypes = (_ordinal == 0 && _assemblySymbol.KeepLookingForDeclaredSpecialTypes); | ||
|
||
|
@@ -691,7 +691,7 @@ internal override CharSet? DefaultMarshallingCharSet | |
internal NamedTypeSymbol LookupTopLevelMetadataTypeWithNoPiaLocalTypeUnification(ref MetadataTypeName emittedName, out bool isNoPiaLocalType) | ||
{ | ||
NamedTypeSymbol? result; | ||
var scope = (PENamespaceSymbol?)this.GlobalNamespace.LookupNestedNamespace(emittedName.NamespaceSegments); | ||
var scope = (PENamespaceSymbol?)this.GlobalNamespace.LookupNestedNamespace(emittedName.NamespaceSegmentsMemory); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is one of the heaviest hitters. we used to break the FQN into N string pieces (each of which copies the data) for the namespace portion of the FQN. Now we just have an array of ROM chunks. The chunks are effectively free (lightweight structs), though we still pay for an array allocation here. Ideally we could remove that as well with a specialized structure. I do see these arrays still show up as about 1% of allocs (the same as before). |
||
|
||
if ((object?)scope == 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.
a lot of hte change will be this mechanical switch from having NamedTypeSymbol/NamespaceSymbol move over to ROM over string.
Of note:
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.
Do we have evidence that customers are allocating
string
to call these APIs? IME these APIs tend to be called through hard coded constants, well known enum values or existingstring
. Not sure we'd save a lot by exposingROM<string>
members (happy to be proved wrong).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.
yeah, i'm basically saying: it's intentional that i'm not pushing for this to be public. and we should only drive that based on evidence that tthere is a need.