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

XArch: Trim the code block to match the actual code length #61523

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

kunalspathak
Copy link
Member

RyuJIT sometimes over-estimate the instruction sizes leading to allocating more code memory than actually utilized. This PR reads the actual code size value and trims the code block array so that the real code size bytes are embedded in the R2RImage.

Related: #57368

@kunalspathak
Copy link
Member Author

@jkotas @davidwrighton @dotnet/jit-contrib

@kunalspathak
Copy link
Member Author

@mangod9

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@kunalspathak kunalspathak changed the title Trim the code block to match the actual code length XArch: Trim the code block to match the actual code length Nov 12, 2021
@kunalspathak
Copy link
Member Author

Also realized that this is only applicable for xarch. For arm64, we also include the size of ro data and hence checking the unused memory is not even possible.

@jkotas
Copy link
Member

jkotas commented Nov 12, 2021

Also realized that this is only applicable for xarch. For arm64, we also include the size of ro data and hence checking the unused memory is not even possible.

Mention this in the comment?

@kunalspathak
Copy link
Member Author

Mention this in the comment?

Sure, I have included some more explanation about armarch.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

The comment at

// JIT to resolve tokens, and make any other callbacks needed to create the code. nativeEntry, and
// nativeSizeOfCode are just for convenience because the JIT asks the EE for the memory to emit code into
// (see code:ICorJitInfo.allocMem), so really the EE already knows where the method starts and how big
// it is (in fact, it could be in more than one chunk).

for compileMethod is no longer quite true, as now the returned nativeEntry has specific semantics... for some platforms.

That's somewhat unsatisfying.

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated Show resolved Hide resolved
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated Show resolved Hide resolved
@kunalspathak
Copy link
Member Author

Failure related to #61548

@BruceForstall - Do you mind taking another look?

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

I'm still not fond of the fact this creates an implicit contract that isn't documented (in corjit.h), and not for all platforms. But ok...

@kunalspathak
Copy link
Member Author

@BruceForstall and me discussed a possible follow-up work of implementing a new JITEE interface for reportMem() that will be useful for crossgen and JIT scenarios.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants