[clr-interp] Add safepoints at method entry and backwards branch #115567
Conversation
Interpreter runs in GC unsafe mode, so without these safepoints for cooperative suspend, the GC could fail to suspend threads executing in the interpreter.
github-actions
bot
added
the
needs-area-label
BrzVlad
added
area-CodeGen-Interpreter-coreclr
and removed
needs-area-label
There was a problem hiding this comment.
Pull Request Overview
Adding safepoints to the interpreter increases GC cooperation by inserting explicit safe points at method entry and on backwards branches. This PR updates several parts of the interpreter:
- Adds CONTRACTL specifications in InterpExecMethod.
- Defines and handles a new INTOP_SAFEPOINT opcode.
- Emits safepoints in the compiler for backwards branches and at method entry.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/interpexec.cpp | Adds CONTRACTL and handles INTOP_SAFEPOINT in the loop |
| src/coreclr/interpreter/intops.def | Registers the new INTOP_SAFEPOINT opcode |
| src/coreclr/interpreter/compiler.cpp | Inserts safepoints during backwards branch processing and at method entry |
Comments suppressed due to low confidence (2)
src/coreclr/interpreter/compiler.cpp:1422
- [nitpick] Consider assessing the performance impact of inserting a safepoint on every backwards branch, especially in tight loops. If the overhead is acceptable by design, a clarifying comment explaining this trade-off could help future maintainers.
if (ilOffset < 0)
src/coreclr/interpreter/compiler.cpp:2122
- [nitpick] Review the insertion of a safepoint at each method entry to ensure it does not introduce unnecessary overhead for short-lived methods. A comment outlining the rationale for this choice would enhance clarity.
AddIns(INTOP_SAFEPOINT);
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
| @@ -2113,6 +2117,10 @@ int InterpCompiler::GenerateCode(CORINFO_METHOD_INFO* methodInfo) | |||
|
|
|||
| codeEnd = m_ip + m_ILCodeSize; | |||
|
|
|||
| // Safepoint at each method entry. This could be done as part of a call, rather than | |||
| // adding an opcode. | |||
| AddIns(INTOP_SAFEPOINT); | |||
There was a problem hiding this comment.
Is there any reason to not to do it as part of the call that made you add the instruction instead?
There was a problem hiding this comment.
Not a very serious reason. Felt cleaner to have a separate instruction for now, given calls in general are an area that will keep suffering modifications in the future.
There was a problem hiding this comment.
Regular CoreCLR JIT expects the safe point to be at the callsite, not in the method entry.
This mismatch between the regular CoreCLR JIT suspension model and interpreter suspension model can lead to GC starvation (GC suspension taking a long time in corner cases). It may be better to inserts the safe point after the call to avoid these subtle issues.
Interpreter runs in GC unsafe mode, so without these safepoints for cooperative suspend, the GC could fail to suspend threads executing in the interpreter.