Repository navigation
Thread.InitializeCurrentThread should be marked as cold code #49520
Description
Thread.InitializeCurrentThread is only called once for the Thread lifetime, however the method it is used in CurrentThread is called lots and every async method has its own call to it (to get ExecutionContext etc)
runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
Lines 328 to 335 in bdd6d35
Currently the asm generated is a conditional jmp forward for it already being set e.g. from AsyncMethodBuilderCore:Start(byref) which is bad for static branch prediction.
G_M49992_IG03:
call [CORINFO_HELP_READYTORUN_STATIC_BASE]
mov rax, gword ptr [rax+24]
test rax, rax
jne SHORT G_M49992_IG05
;; bbWeight=1 PerfScore 6.25
G_M49992_IG04:
call [Thread:InitializeCurrentThread():Thread]
;; bbWeight=0.25 PerfScore 0.75
G_M49992_IG05:
mov ...Reversing the condition in CurrentThread
Thread? currentThread = t_currentThread;
if (currentThread is not null)
{
return currentThread;
}
return InitializeCurrentThread();While it generates a conditional jmp forward for InitializeCurrentThread it also then adds a jmp for the regular path:
G_M49992_IG03:
call [CORINFO_HELP_READYTORUN_STATIC_BASE]
mov rax, gword ptr [rax+24]
test rax, rax
je SHORT G_M49992_IG05
;; bbWeight=1 PerfScore 6.25
G_M49992_IG04:
jmp SHORT G_M49992_IG06
;; bbWeight=0.50 PerfScore 1.00
G_M49992_IG05:
call [Thread:InitializeCurrentThread():Thread]
;; bbWeight=0.50 PerfScore 1.50
G_M49992_IG06:
mov ...Marking it as a cold method should resolve this by moving the call to InitializeCurrentThread to the end of the method so the regular path didn't have to jump over it?
/cc @EgorBo
category:performance
theme:block-layout
Activity
AndyAyersMS commentedon Mar 12, 2021
My understanding is that static prediction is out of favor these days.
But reordering like you propose has other benefits:
benaadams commentedon Mar 12, 2021
Yes as normally the branch predictor will be the dominant factor; however the peculiarity here is its included in every async method's kick off; as they all have different generic types for
ref TStateMachineso it will be more common that these calls are "fresh" as far as the branch predictor history is aware as its in so many places? (TheMoveNextmethods less so since they go viaExecutionContext.Runwhich callsCurrentThreadso is a common code point)Being more generic if there are more examples could add another enum to
MethodImplOptions? e.g.ColdCodebenaadams commentedon Mar 12, 2021
PGO could obviously also address this; however it seems problematic to always start in the "wrong" state for all these methods?
jkotas commentedon Mar 12, 2021
We should be able to start in a good state with static pre-generate PGO data.
EgorBo commentedon Mar 12, 2021
Maybe it's time to introduce Likely/Unlikely APIs?
jkotas commentedon Mar 12, 2021
Yes, these APIs would make these micro-optimizations to be done manually. I do not think we would welcome PRs to add likely/unlikely annotations in the core libraries. I think we would rather want to depend on the static PGO data instead since it scales a lot better and it does not require humans to guess what is more likely.
AndyAyersMS commentedon Mar 13, 2021
Here you can see PGO doing this sort of thing -- I'll see if I can dig up an example with
InitializeCurrentThreadAndyAyersMS commentedon Mar 13, 2021
Here it is -- Note SPMI hides the callee name.
This is with dynamic PGO, but static PGO will do similar things.
Raw profile view showing branch bias:
benaadams commentedon Mar 13, 2021
That's beautiful! 🤩
Should this issue be closed?
AndyAyersMS commentedon Mar 15, 2021
We should be seeing profile data flowing into both prejitting and jitting soon, so it would be good to confirm once that happens that we get the above without any explicit enabling of PGO.
So let's keep this open as a reminder.
AndyAyersMS commentedon Mar 19, 2021
Here's a listing from today's build. We now have PGO data flowing into SPC and on through to jitting.
This is happening without any COMPlus settings.
benaadams commentedon Mar 19, 2021
Does it pick that up for async Method kick offs (as they will be new inlines); though presumably that will kick in at Tier1 anyway and at Tier0 the ordering of that
ifprobably isn't a significant featureAndyAyersMS commentedon Mar 25, 2021
We still have some work to do here, in cases where the caller doesn't have PGO data.
For instance
The inlinee
Thread:get_CurrentThread():Threadhas good looking profile data initially:but the caller does not, and so we don't bother to scale the inlinee counts, and after the resultant block fusion and unity weight descaling we end up seriously confused:
Looks like we should always compute a scale factor if the callee has PGO.
In general the jit is not very smart yet about mixing PGO and non-PGO.
benaadams commentedon Mar 25, 2021
Would this currently be an issue as if the caller has a loop as it wouldn't do a retiering Jit?
AndyAyersMS commentedon Mar 25, 2021
With the advent of #49793 many framework methods will come equipped with profile data. And with the advent of #47558 the runtime will always tell the jit to look for profile data for optimized methods.
Thus for any method that inlines
Thread:get_CurrentThread()and is optimized the jit will see the profile data and be able to tell that the branch is biased. You can see that in the profile incorporation above, before the jit decides to spoil things...