Separate code manager for the interpreter #112985
Conversation
There was a problem hiding this comment.
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.
|
cc: @BrzVlad |
|
This conflicts with #111759 because cDAC starts to use the |
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. |
|
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.
@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? |
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.
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. |
|
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. |
This change adds a separate code manager for the interpreted code. Here is an overview of the changes: * A base class EECodeGenManager is extracted from the EEJitManager * A new class InterpreterJitManager derived from the EECodeGenManager is added and implemented. * Similarly, a base class CEECodeGenInfo is extracted from the CEEJitInfo * A new class CInterpreterJitInfo derived from the CEECodeGenInfo is 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 is 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
force-pushed
the
interpreter-code-manager
branch
from
e028232 to
9fc72c4
Compare
janvorli
force-pushed
the
interpreter-code-manager
branch
2 times, most recently
from
90e877c to
4ab314a
Compare
Make the jit and interpreter code headers independent
janvorli
force-pushed
the
interpreter-code-manager
branch
from
4ab314a to
41c278f
Compare
janvorli
force-pushed
the
interpreter-code-manager
branch
from
1361187 to
fc7b33b
Compare
|
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. |
|
I have reworked it and got rid of all of the regression. I will update this PR on Monday |
The previous way was introducing 5% regression in all exception handling microbenchmarks, this gets us back to the same perf as without any code manager changes.
The clang doesn't do late parsing of the template code, so the helpers cannot be in the .inl file as they reference some types that are not defined yet at that point.
|
@jkotas can you please take another look? The code headers are now completely separated. |
| else | ||
| #endif // FEATURE_INTERPRETER | ||
| { | ||
| #if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) |
| startInfo.EnvironmentVariables["DOTNET_AltJit"] = "RunInterpreterTests"; | ||
| startInfo.EnvironmentVariables["DOTNET_InterpreterName"] = libInterp; | ||
| startInfo.EnvironmentVariables["DOTNET_InterpreterPath"] = Path.Combine(coreRoot, libInterp); | ||
| startInfo.EnvironmentVariables["DOTNET_Interpreter"] = "RunInterpreterTests"; |
There was a problem hiding this comment.
When using the AltJit option I noticed I had problems unless I passed all these flags. Shouldn't as of this change be enough to just pass the DOTNET_Interpreter, while the rest could be omitted ?
There was a problem hiding this comment.
Right, it should be sufficient. Let me get rid of the other env variables setting in the test.
|
@jkotas I have realized I have not realized I also need to add an InterpreterCodeManager (ICodeManager derived one), so I am working on adding a one to this PR. |
|
BTW: I do not think that the split between IJitManager and ICodeManager is very sensible. If we fine a good reason to, I think it would be fine to merge the two into a single interface. |
|
It seems it would make sense. That's also why I have originally missed the ICodeManager. But let's not complicate this PR by such a change, we can do it as a follow up change. |
|
|
||
| // CEEJitInfo is the concrete implementation of callbacks that the EE must provide for the JIT to do its | ||
| // work. See code:ICorJitInfo#JitToEEInterface for more on this interface. | ||
| class CEEJitInfo : public CEEInfo | ||
| class CEEJitInfo : public CEECodeGenInfo |
There was a problem hiding this comment.
Nit, I think you can use class CEEJitInfo final : public CEECodeGenInfo instead of scattering final throughout the definition of CEEJitInfo. Also the same for CInterpreterJitInfo
This change adds a separate code manager for the interpreted code. Here is a high level overview of the changes:
EECodeGenManagerwas extracted from theEEJitManagerInterpreterJitManagerderived from theEECodeGenManagerwas added and implemented.CEECodeGenInfowas extracted from theCEEJitInfoCInterpreterJitInfoderived from theCEECodeGenInfowas added and implementedCodeHeaderandRealCodeHeaderbecame base classes andJitCodeHeader,RealJitCodeHeader,InterpreterCodeHeaderandRealInterpreterCodeHeaderwere added. TheCodeHeaderderivates don't add any extra data, theJitCodeHeaderjust provides the methods that are needed to access Jit specific members of theRealJitCodeHeader.UnsafeJitFunctionwas refactored so that the interpreter (if enabled) is invoked first and then the AltJit / Jit ones are called.DOTNET_Interpreter,DOTNET_InterpreterPathandDOTNET_InterpreterNameenv vars are added to match the existing AltJit ones.