Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improve the performance of the type loader through various tweaks #85743
Improve the performance of the type loader through various tweaks #85743
Changes from 19 commits
ff0e821
3b8143c
0c88982
910f4c4
418af3c
c1c9983
48dfa0e
d274c51
5bae879
9f9e586
f5e72a6
8eaf4a0
ac7ec65
1a57a0a
5f770c6
52bd220
dfecc6f
6ef05ec
eff81d5
db08221
3b21879
06b6ffd
df3a418
75144c4
1f44575
e6c0b4b
0f45839
2648965
bc127bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Can we do this in a constructor? This is already a
RuntimeXXXDesc
so I wouldn't mind if the constructor just understands aReadOnlySpan
ofRuntime.GenericVariance
.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.
Refactor requested and implemented.
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.
Isn't this a bug if someone asks for these? Signature variables don't have "a finalizer" but also if someone is asking for that, maybe it's a bug. I've ran into the assert not setting this flag produces a 100% of times this was a bug in my code that I was happy to find so easily.
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.
Unfortunately, this work operates almost entirely over uninstantiated types and methods, and has a few spots where it actually needs to use these things. However, it might be the case that I can only set a minimal number of flags. I enabled them all as it seemed like the right thing to do, but I believe the only 1 I really needed to work was the attribute cache.
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.
What's the stack that leads to this being needed? This really feels like something where the only right answer is to unask the question.
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 don't think we need the Volatile here.
If this is perf critical, we can use the same pattern as BaseType:
runtime/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs
Lines 174 to 182 in 8ee61fe
If it's not perf critical maybe we don't even need to cache it.
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 don't know much about the ILVerify codebase, but doesn't this need
InstantiateSignature
?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.
Maybe? I don't know that codebase either, but it doesn't seem unreasonable.