Skip to content
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

R2R ilstubs #24823

Merged
merged 20 commits into from Jun 11, 2019
Merged

R2R ilstubs #24823

merged 20 commits into from Jun 11, 2019

Conversation

@davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented May 29, 2019

IL stub generation for interop takes measurable time at startup, and it is possible to generate some of them in an ahead of time

This change introduces ahead of time R2R compilation of IL stubs

  • Relatively complete coverage for System.Private.CoreLib
  • Limited IL stubs for other assemblies
    • Only stubs which do not use any api surface in their marshalers.

As of this writing IL stubs do not participate in tiering. Need for this will be investigated before checkin

davidwrighton added 18 commits May 26, 2019
- Restrict generation of IL stubs into R2R binaries where the IL stub may have runtime specific dependencies
- Enable various marshallers
- Allow/disallow a few different instructions
- Also tweak logic around corelib ilstub r2r usage so that it actually uses the ilstubs in only the right circumstances
- The cross bitness logic is not correct for IL Stub generation
@davidwrighton davidwrighton changed the title [WIP] R2R ilstubs R2R ilstubs Jun 10, 2019
@davidwrighton davidwrighton marked this pull request as ready for review Jun 10, 2019
src/inc/corcompile.h Show resolved Hide resolved
src/vm/compile.cpp Show resolved Hide resolved
src/vm/compile.cpp Outdated Show resolved Hide resolved
src/vm/compile.cpp Show resolved Hide resolved
src/vm/prestub.cpp Outdated Show resolved Hide resolved
@@ -1324,19 +1324,44 @@ BOOL ZapSig::EncodeMethod(
{
Module * pReferencingModule = pMethod->IsNDirect() ?
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Jun 10, 2019

Choose a reason for hiding this comment

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

Please document why pReferencingModule is valid to be NULL. Based on existing code this is a core assumption that is no longer value. If there is a way I would strong suggest to remove using NULL as an invariant since this function was written where that was never an issue.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Jun 10, 2019

Choose a reason for hiding this comment

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

In particular see previous logic at line 1342.

Copy link
Member Author

@davidwrighton davidwrighton Jun 11, 2019

Choose a reason for hiding this comment

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

The problem is that there isn't a referencing module. Dynamic methods don't really have associated modules in the same way that normal methods do. (In particular, the tokens are not relative to the module, which is what all of the code here is assuming.)

Copy link
Member Author

@davidwrighton davidwrighton Jun 11, 2019

Choose a reason for hiding this comment

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

But a comment is certainly worth it.

}
else
{
// Dynamic scope case. IL Stubs only
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Jun 10, 2019

Choose a reason for hiding this comment

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

Any way we can assert this instead of commenting on it? (i.e. "IL Stubs only" <- can that be asserted?)

ThrowHR(E_FAIL);
}

if (pInfoModule == pMethod->GetModule())
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Jun 10, 2019

Choose a reason for hiding this comment

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

Is this checking where the method's metadata is located? This check warrants a comment I think.

src/zap/zapinfo.cpp Outdated Show resolved Hide resolved
jkotas
jkotas approved these changes Jun 10, 2019
@davidwrighton davidwrighton merged commit 93675fb into dotnet:master Jun 11, 2019
2 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants