-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Faster type load for scenarios made more common by generic math #54588
Conversation
How does this impact startup? Does it remove the regression that will get introduced? |
This fixes 90% of the startup slowdown introduced by the generic math feature. Of course, it also doesn't quite work as can be seen from the tests, but it looks like its a feasible direction. The other thing is, there is a runtime perf penalty which isn't shown here. Of particular concern is the change to interface casting. |
9aa4be4
to
3e329f9
Compare
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.
Looks great to me, thank you! I have only noticed a few formatting nits, no functional issues.
if (uninstGenericCase && pItfPossiblyApprox->HasInstantiation() && pItfPossiblyApprox->ContainsGenericVariables()) | ||
{ | ||
// We allow a limited set of interface generic shapes with type variables. In particular, we require the instantiations to be exactly type variables | ||
// the current type |
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.
Nit - the two lines of this code comment seem somewhat disjoint, is there a missing "of" between the lines?
src/coreclr/vm/siginfo.cpp
Outdated
@@ -989,7 +989,8 @@ TypeHandle SigPointer::GetTypeHandleThrowing( | |||
BOOL dropGenericArgumentLevel/*=FALSE*/, | |||
const Substitution * pSubst/*=NULL*/, | |||
// ZapSigContext is only set when decoding zapsigs | |||
const ZapSig::Context * pZapSigContext) const | |||
const ZapSig::Context * pZapSigContext, | |||
MethodTable *pMTInterfaceMapOwner) const |
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.
Nit - indentation?
// to have found all of the interfaces that the type implements, and to place them in the interface list itself. Also | ||
// we can assume no ambiguous interfaces | ||
// Code related to this is marked with #SpecialCorelibInterfaceExpansionAlgorithm | ||
if (!(GetModule()->IsSystem() && IsValueClass())) |
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.
Nit - indentation
1. For interface maps defined in System.Private.CoreLib, rely on the C# compiler to prevent any ambiguous duplicates, and to find the full interface expansion, instead of expanding it within the type loader See code marked with #SpecialCorelibInterfaceExpansionAlgorithm 2. For interface map expansion that follows the curiously recurring generic pattern, place the open instantiation of the type in the interface map instead of the the exact instantiation, and update all places in the runtime which consider the interface map to deal with that change (Mostly by being able to not resovle to an exact interface, but in some cases there is a helper which will do exactly that)
- Re-enable fast path for NonTrivialCasting for interface misses - Adjust SetInterface usage to only occur for scenarios where the found type if fully loaded - Only use #SpecialCorelibInterfaceExpansionAlgorithm on ValueTypes, turns out derived types have all the fun problems - Adjust casting logic to work with the approx interface and owner pair. This avoid the need to load types in CanCastTo, which is important as CanCastTo is used heavily during type load in scenario where further type loading is prohibited - Give checks for the special marker type a unique name instead of just re-using IsGenericTypeDefinition. Use IsSpecialMarkerTypeForGenericCasting instead.
3e329f9
to
1a32f13
Compare
…bugger2 * origin/main: (78 commits) Fix unreached during dump. (dotnet#54861) Fix lowering usage of an unset LSRA field. (dotnet#54731) Fix setting breakpoints on AVX 256 instructions and other 32 byte immediate instructions (dotnet#54786) Add perf_slow yaml (dotnet#54853) Faster type load for scenarios made more common by generic math (dotnet#54588) Make sure we consider buffer length when marshalling back Unicode ByValTStr fields (dotnet#54695) Add YieldProcessor implementation for arm (dotnet#54829) Remove ActiveIssue for dotnet#50968 (dotnet#54831) Enable System.Text.Json tests for Wasm AOT (dotnet#54833) Remove ActiveIssue for dotnet#51723 (dotnet#54830) Fix load exception on generic covariant return type (dotnet#54790) Obsolete X509Certificate2.PrivateKey and PublicKey.Key. (dotnet#54562) First round of converting System.Drawing.Common to COMWrappers (dotnet#54636) Fix alloc-dealloc mismatches (dotnet#54701) Add one-shot ECB methods [Mono] MSBuild Task housekeeping (dotnet#54485) Move iOS/tvOS simulator AOT imports in the Mono workload (dotnet#54821) Remove unnecessary char[] allocation from Uri.GetRelativeSerializationString (dotnet#54799) Reduce overhead of Enumerable.Chunk (dotnet#54782) Fix EnumMemberRefs always returning NULL (dotnet#54805) ...
Change interface map layout in two interesting ways
Performance of launching an extremely simple .NET process (process with empty main method). Results acquired using local testing on my developer machine, using a simple script that launches the process 500 times in a row.