I am surprised that you were able to get away with the extra indirection in DispatchStubCode. It will be interesting to see the results of the microbenchmark runs.

I am not sure what indirection you mean. This was the original code:

I meant the performance overhead from the extra indirections introduced by this refactoring. The original code had two immediates, the new code has two indirections.

@adamsitnik What's the best way to do dotnet/performance benchmark run for a PR like this one to see what improved/regressed on different target platforms?

Ah, got it, I've thought you meant it somehow the other way.

What's the best way to do dotnet/performance benchmark run for a PR like this one to see what improved/regressed on different target platforms?

We have two options:

1. Run the benchmarks using two different coreruns (before & after) and store them in dedicated folders, then compare them using ResultsComparer tool:https://github.com/dotnet/performance/blob/ef3edbc52d92c6b30ba1c316082f46865e5ff1d6/docs/benchmarking-workflow-dotnet-runtime.md#preventing-regressions It's the best solution if the implementation might change and you might need to re-run the benchmarks using updated corerun.
2. Run the benchmarks using two different coreruns without storing them in dedicated dolders, use BDN to perform the statistical test on the fly:https://github.com/dotnet/performance/blob/03207f183f042f6fc6b9f341df7a0e36b7175f5d/src/benchmarks/micro/README.md#private-runtime-builds it's good if you have only few benchmarks or don't intend to change the implementation

I think this condition is not right for the tiered compilation disabled case (not default, but still used by some first parties). Does this need to call into code version manager?

It is possible (although all the tests pass with tiered compilation disabled except for one where the failure looks unrelated to this). What do you think is the specific problem here?

I think that the tiered compilation disabled case will go through unnecessary indirection when the target method is JITed already.

I've tried to add!ftn->HasStableEntryPoint() to the condition. It severely degraded the plaintext benchmark performance. I've also thought that!ftn->IsPointingToStableNativeCode() might be needed, but the function seems to never report true together with the preexisting condition (based on my testing when I've added an assert and none of the coreclr pri1 tests hit it, with or without tiered compilation enabled).

I think the logic should be something like this:

if (ftn->IsVersionable()){    IAT_PVALUE // must go via indirection to enable versioning }else{    if (the target has final code)        IAT_VALUE for the final code // this includes FCalls and methods that are already JITed (w/o tiering)    else        IAT_PVALUE // must go via indirection to pick up the final code once it is ready}

I will give it a try

What guarantees that we have the final code of the method by this point?

If this is a valid optimization, we should be doing it when fillinghlpDynamicFuncTable.

We don't need to have the final code. This just passes the fixup precode indirection slot to the JIT. If we don't have the final code, the slot points to the fixup part of the slot. When we have the JITted code, the slot points to it. Without this optimization, JIT would jump to the beginning of the fixup stub and the indirect jump in there would jump through the slot. So we save one jump by this. I have found this using the performance repo microbenchmarks where the casting benchmarks were consistently slower with my change. With this fix, they are now consistently faster.

We can modify thehlpDynamicFuncTable entry to be able to carry either the indirection or the target address and a flag indicating which one it is. I was going back and forth in my head whether to do that or do it the way I've ended up doing it.

However, there is a_AddrIsJITHelper function on 32 bit Windows in the debugger controller code that compares addresses of all the helpers with an address that the debugger has stepped into to avoid the debugger stopping in unmanaged runtime code. While I believe we should not call this methods that code for prefix stubs (we should get TRACE_STUB tracetype), I need to double check that. I am talking about the case when the helper was not JITted yet and so the indirection would go to the middle of the fixup stub and the code there would not detect it was in a helper.

This just passes the fixup precode indirection slot to the JIT. If we don't have the final code, the slot points to the fixup part of the slot.

Ah ok, I have missed that.