Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Do not track RuntimeDeterminedMethod dependencies from canonical methods - #5040

Merged
MichalStrehovsky merged 4 commits into
dotnet:masterfrom
MichalStrehovsky:avoidRuntimeDetermined2
Dec 1, 2017
Merged

Do not track RuntimeDeterminedMethod dependencies from canonical methods#5040
MichalStrehovsky merged 4 commits into
dotnet:masterfrom
MichalStrehovsky:avoidRuntimeDetermined2

Conversation

@MichalStrehovsky

Copy link
Copy Markdown
Member

The concept of RuntimeDeterminedMethodNode was added when I was trying to support a mode where generic dictionaries could have "holes" (null entries for things that are not used for the particular instantiation). This turned out to be not feasible and we now have dependency analysis infra to patch those holes up anyway. We don't need to track dependencies of canonical code on such granularity.

The only exception to that were generic methods, but I'm fixing that by having the method generic dictionary (which is tracked as a runtime determined dependency of the canonical body) to also depend on the ShadowConcreteMethod (to make sure we can actually fill it).

I'm doing this because the devirtualization in RyuJIT causes getCallInfo to be called with bogus pResolvedToken and we can no longer use that to get a RuntimeDeterminedMethod anyway.

The concept of RuntimeDeterminedMethodNode was added when I was trying to support a mode where generic dictionaries could have "holes" (null entries for things that are not used for the particular instantiation). This turned out to be not feasible and we now have dependency analysis infra to patch those holes up anyway. We don't need to track dependencies of canonical code on such granularity.

The only exception to that were generic methods, but I'm fixing that by having the method generic dictionary (which is tracked as a runtime determined dependency of the canonical body) to also depend on the ShadowConcreteMethod (to make sure we can actually fill it).

I'm doing this because the devirtualization in RyuJIT causes `getCallInfo` to be called with bogus `pResolvedToken` and we can no longer use that to get a `RuntimeDeterminedMethod` anyway.
@MichalStrehovsky

Copy link
Copy Markdown
Member Author

@A-And this fixes the first issue you were seeing with compiling the plain dotnet new web result with optimizations enabled.

@MichalStrehovsky

MichalStrehovsky commented Nov 29, 2017

Copy link
Copy Markdown
Member Author

@davidwrighton PTAL

This causes an unfortunate 2 kB size on disk regression around Unsafe intrinsics - e.g. Unsafe.Add<T> for T == __Canon gets a generic dictionary passed as a hidden argument at the callsite, but the generic dictionary is actually unused. Scanner computed that the dictionary also needs the generic dictionary of Unsafe.SizeOf for that T. The code I'm newly adding now triggers generation of a standalone method body for Unsafe.SizeOf<__Canon> (dictionary depends on the shadow method, which depends on the canonical method).

Seems like the fix would be to investigate why RyuJIT doesn't inline the Add, despite the AggressiveInlining hint.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

Let's close for now. Turns out this is actually more involved.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

Ok, I now have a complete fix. This uncovered a problem where the infrastructure that tracks dictionary dependencies was unaware of the methods injected into delegate types.

We had a rule that said: whenever someone injects a virtual method on a type, the type system context needs to know. The rule needs an addition that says: whenever someone injects a method that contributes to a generic dictionary, type system needs to know too.

@davidwrighton davidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@MichalStrehovsky
MichalStrehovsky merged commit f826d56 into dotnet:master Dec 1, 2017
@MichalStrehovsky
MichalStrehovsky deleted the avoidRuntimeDetermined2 branch December 1, 2017 19:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants