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

Separate code manager for the interpreter #112985

Merged
merged 16 commits into from
Mar 22, 2025

Conversation

janvorli
Copy link
Member

This change adds a separate code manager for the interpreted code. Here is a high level overview of the changes:

  • A base class EECodeGenManager was extracted from the EEJitManager
  • A new class InterpreterJitManager derived from the EECodeGenManager was added and implemented.
  • Similarly, a base class CEECodeGenInfo was extracted from the CEEJitInfo
  • A new class CInterpreterJitInfo derived from the CEECodeGenInfo was added and implemented
  • The CodeHeader and RealCodeHeader became base classes and JitCodeHeader, RealJitCodeHeader, InterpreterCodeHeader and RealInterpreterCodeHeader were added. The CodeHeader derivates don't add any extra data, the JitCodeHeader just provides the methods that are needed to access Jit specific members of the RealJitCodeHeader.
  • The UnsafeJitFunction was refactored so that the interpreter (if enabled) is invoked first and then the AltJit / Jit ones are called.
  • A new DOTNET_Interpreter, DOTNET_InterpreterPath and DOTNET_InterpreterName env vars are added to match the existing AltJit ones.

@janvorli janvorli added this to the 10.0.0 milestone Feb 27, 2025
@janvorli janvorli self-assigned this Feb 27, 2025
@Copilot Copilot bot review requested due to automatic review settings February 27, 2025 14:03

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a separate code manager dedicated to the interpreter by extracting base classes and adding new interpreter-derived classes for code generation and management.

  • Extracted EECodeGenManager and CEECodeGenInfo base classes; added InterpreterJitManager and CInterpreterJitInfo.
  • Updated headers to include interpreter-specific versions alongside JIT ones.
  • Revised test configuration to use new interpreter environment variables instead of AltJit ones.

Reviewed Changes

File Description
src/tests/JIT/interpreter/InterpreterTester.cs Updates the test environment variables to use interpreter-specific keys, ensuring consistency with the new code manager changes.

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

@janvorli
Copy link
Member Author

cc: @BrzVlad

@max-charlamb
Copy link
Contributor

This conflicts with #111759 because cDAC starts to use the RealCodeHeader offsets for nUnwindInfos and unwindInfos that this PR moves to the JitRealCodeHeader. The cDAC does not currently support the interpreter so I think it only cares about the JitRealCodeHeader right now and we should change the datadescriptors to use that instead of the new base class. This would be a breaking cDAC change and require the DataType name to change on the managed side too, but there isn't any impact.

https://github.com/dotnet/runtime/pull/111759/files#diff-d10a483484c15e47b5c1d129ff70b1bb88ad775e8f113475f8789aad1a4ffad5

@davidwrighton

@jkotas
Copy link
Member

jkotas commented Feb 27, 2025

This conflicts with #111759 because cDAC starts to use the RealCodeHeader offsets for nUnwindInfos and unwindInfos that this PR moves to the JitRealCodeHeader

My preference would be to solve this by avoiding coupling between the JIT code manager and interpreter code manager. Leave the existing code header for JITed code alone, and define a dedicated code header for interpreted code. There is going to be some duplication for now and that's fine. The advantage is that it allows each of them to evolve independently.

@janvorli
Copy link
Member Author

I am not sure I understand why cdac depends on the name of the code header. The JitCodeHeader / RealJitCodeHeader in my PR are not different from the old CodeHeader / RealCodeHeader, they are just renames of the old ones.

My preference would be to solve this by avoiding coupling between the JIT code manager and interpreter code manager.

@jkotas do you mean instead of the shared EECodeGenManager base class to have the InterpreterJitManager a copy of almost all of the code from the EEJitManager?

@jkotas
Copy link
Member

jkotas commented Feb 27, 2025

I am not sure I understand why cdac depends on the name of the code header

cdac infrastructure expects the data contract type representation to match the implementation. If you split a type into two types without actually changing the layout, it changes the data contract. You are right that the contract type representation does not have to match the implementation. There is probably a way how to convince the cdac infrastructure to deal with the divergence, but it is not how it is usually done. I am not sure whether we have a prior example.

do you mean instead of the shared EECodeGenManager base class to have the InterpreterJitManager a copy of almost all of the code from the EEJitManager?

I am concerned about the code header. I think we want the interpreter bytecode header to be decoupled from JITed code header, so we are free to change one in any way we want without impacting the other.

I do not have a strong opinion whether it should be implemented as inheritance hierarchy, or as two types calling into static helpers to facilitate code sharing.

@janvorli
Copy link
Member Author

Ah, so the concern is just the code header and the code that works with it, that makes sense. I also think we may eventually diverge the interpreter and jit code header. I was thinking you were talking about all of the code in the code manager.

@janvorli janvorli force-pushed the interpreter-code-manager branch from e028232 to 9fc72c4 Compare February 28, 2025 02:01
@janvorli janvorli force-pushed the interpreter-code-manager branch 4 times, most recently from 1361187 to fc7b33b Compare February 28, 2025 17:50
@janvorli
Copy link
Member Author

I've tried to run the dotnet/performance microbenchmarks for EH. All of them are about 5% worse with the last change that separates the code headers completely. With the initial change, the difference was < 2%. I need to think about some other way to solve the code header separation.

@janvorli
Copy link
Member Author

I have reworked it and got rid of all of the regression. I will update this PR on Monday

@janvorli
Copy link
Member Author

janvorli commented Mar 4, 2025

@jkotas can you please take another look? The code headers are now completely separated.

* Refactored GC and EH info allocation
* Added Interpreter-TODO at some places
* Reverted some changes that were not needed anymore
@janvorli janvorli force-pushed the interpreter-code-manager branch from 903fea8 to f690f1d Compare March 19, 2025 21:41
@janvorli
Copy link
Member Author

@jkotas I have updated the PR based on your feedback.

* Get rid of some template usages by small refactoring
@janvorli
Copy link
Member Author

/ba-g the wasm failure is unrelated and it is a known infra issue.

@janvorli janvorli merged commit e206974 into dotnet:main Mar 22, 2025
99 of 101 checks passed
@janvorli janvorli deleted the interpreter-code-manager branch March 22, 2025 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants