[clr-interp] Ensure initialization of MethodDesc->m_interpreterCode - #130026
#130026[clr-interp] Ensure initialization of MethodDesc->m_interpreterCode#130026janvorli merged 1 commit intodotnet:maindotnet/runtime:mainfrom BrzVlad:fix-clrinterp-code-race-halfBrzVlad/runtime:fix-clrinterp-code-race-halfCopy head branch name to clipboard
Conversation
When calling a method, we need to determine whether the method is interpreted or not. If `m_interpreterCode` is uninitialized, we will call `ThePrestub` to trigger method compilation (PrepareInterpreterCode). The expectation is that, once `ThePrestub` finishes, the interpreter code will always be initialized. This assumption didn't hold with the current code in the following scenario: Two threads can race to compile a method in `JitCompileCodeLocked` (since method compilation is not always done under a lock in order to prevent deadlocks from recursive dependencies). Following method compilation the code publishing flow is to set the native code first via a CAS in `SetNativeCode` followed by a simple publish of the interpreter code by the thread which wins the native code publish race. The problem with this approach is that the thread losing the race can return early, without having the interpreter code set. We fix this by making sure the interpreter code is always published by all threads compiling the method. It shouldn't matter which thread wins the race because both compilation results are identical and they are both preserved.
|
Tagging subscribers to this area: @JulieLeeMSFT, @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Pull request overview
This change adjusts the CoreCLR method compilation publishing sequence so that, for interpreted methods, MethodDesc::m_interpreterCode is always initialized even when multiple threads race in JitCompileCodeLocked and one thread loses the native code publish CAS.
Changes:
- Move the
SetNativeCodepublish/early-return block to occur after publishing interpreter bytecode (SetInterpreterCode) so the losing thread still initializesm_interpreterCodebefore returning the winner’spOtherCode.
|
I believe there is still a theoretical issue. One thread is publishing the interpreter code followed by the native code. Then another thread might not bother compiling because it exits early via runtime/src/coreclr/vm/prestub.cpp Line 570 in 0bc7eda In that case I believe it is possible for the thread to obtain a stale value of the interpreter code. Meaning we would need a store barrier between setting of interpreter and native codes and then a load barrier between checking that the native code is set and the future loading of the interpreter code. Anyhow, I'm not able to reliably reproduce this issue for now, so I'll wait to see if we are still getting this assertion in the future. |
…130026) When calling a method, we need to determine whether the method is interpreted or not. If `m_interpreterCode` is uninitialized, we will call `ThePrestub` to trigger method compilation (PrepareInterpreterCode). The expectation is that, once `ThePrestub` finishes, the interpreter code will always be initialized. This assumption didn't hold with the current code in the following scenario: Two threads can race to compile a method in `JitCompileCodeLocked` (since method compilation is not always done under a lock in order to prevent deadlocks from recursive dependencies). Following method compilation the code publishing flow is to set the native code first via a CAS in `SetNativeCode` followed by a simple publish of the interpreter code by the thread which wins the native code publish race. The problem with this approach is that the thread losing the race can return early, without having the interpreter code set. We fix this by making sure the interpreter code is always published by all threads compiling the method. It shouldn't matter which thread wins the race because both compilation results are identical and they are both preserved. Should fix sporadic assertions from #129465
When calling a method, we need to determine whether the method is interpreted or not. If
m_interpreterCodeis uninitialized, we will callThePrestubto trigger method compilation (PrepareInterpreterCode). The expectation is that, onceThePrestubfinishes, the interpreter code will always be initialized. This assumption didn't hold with the current code in the following scenario:Two threads can race to compile a method in
JitCompileCodeLocked(since method compilation is not always done under a lock in order to prevent deadlocks from recursive dependencies). Following method compilation the code publishing flow is to set the native code first via a CAS inSetNativeCodefollowed by a simple publish of the interpreter code by the thread which wins the native code publish race. The problem with this approach is that the thread losing the race can return early, without having the interpreter code set.We fix this by making sure the interpreter code is always published by all threads compiling the method. It shouldn't matter which thread wins the race because both compilation results are identical and they are both preserved.
Should fix sporadic assertions from #129465