Remove extra UnmanagedCallersOnly overhead on x86 #46238

jkoritzinsky · 2020-12-19T00:24:01Z

This PR teaches the JIT to understand how to emit a method with a native x86 calling convention and then utilizes that feature to remove the argument shuffling stubs for both Windows and Unix x86 Reverse P/Invoke thunks.

As part of this work, the JIT helpers for Reverse P/Invokes now report transitions to the profiler and the debugger as the x86 Reverse P/Invoke thunk did.

This allows us to remove the rest of the overhead that x86 has for Reverse P/Invokes and UnmanagedCallersOnly methods in comparison to other platforms.

Since I changed the signature of the JIT_ReversePInvokeEnter helper, I tentatively changed the JIT-EE interface ID. If we don't need to do so for this change, we can revert that part of this change.

I've validated that there are no crossgen diffs. The results from a framework crossgen JIT diff on Windows x86 are included below.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 26561274
Total bytes of diff: 26561274
Total bytes of delta: 0 (0.00% of base)

0 total files with Code Size differences (0 improved, 0 regressed), 267 unchanged.

0 total methods with Code Size differences (0 improved, 0 regressed), 230772 unchanged.
Found 18 files with textual diffs.

Fixes #33582
Fixes #46177
Fixes #47037

…he correct argument order and register usage on x86.

… to do argument shuffling.

…helper to match custom x86 thunk. Fixes dotnet#46177

…ve Windows x86 unmanaged callers only to not have extra overhead and put reverse P/Invoke stubs for Windows x86 on the UnmanagedCallersOnly plan.

…ntime; branch 'master' of github.com:dotnet/runtime into x86-reverse-stub-cleanup

…llersOnly has been simplified.

src/coreclr/vm/jithelpers.cpp

src/coreclr/inc/jiteeversionguid.h

src/coreclr/vm/jithelpers.cpp

jkotas · 2020-12-19T10:02:39Z

It would be useful to collect some performance numbers: marshalled reverse P/Invoke no-op delegate, UnmanagedCallersOnly no-op, on both x86 and x64.

I love the simplification!

…emitted unmanaged thunk stub, we need to handle the x86 differences for copy-constructed parameters in the IL stub.

jkoritzinsky · 2020-12-21T22:03:43Z

I've started taking a look at the test failures. The Vector2_3_4 test failure is due to CodeGen::genSIMDSplitReturn in the JIT not correctly handling the case where we are returning a Vector2 in EAX:EDX. It attempts to emit the following instructions:

IN001e:        vmovaps  eax, xmm0
IN001f:        vmovaps  edx, eax
IN0020:        vshufpd  xedx, xedx, 1

However, these instructions are incorrect. The actually emitted instructions are:

17EE382C  vmovaps     xmm0,xmm0  
17EE3830  vmovaps     xmm2,xmm0  
17EE3834  vshufpd     xmm2,xmm2,xmm2,1

As a result, the return value isn't put into the correct registers and the test fails.

We've had a number of these types of bugs when doing this work for x86. @sandreenko can you help out with fixing this one after you fix the bug I created the last time I tried to fix this type of bug?

This reverts commit 423f70e.

…ate a new stack frame.

…on the stack.

jkoritzinsky · 2020-12-24T19:17:42Z

I've validated as of the last commit (69e14c6) that there are still no JIT diffs on x86.

src/coreclr/vm/dllimportcallback.cpp

src/coreclr/jit/flowgraph.cpp

sandreenko

Jit changes LGTM with 1 qustion

src/coreclr/inc/readytorun.h

AaronRobinsonMSFT

LGTM.

The one final thing I would like to confirm validation for is the IJW scenario. I would think that we have existing tests for this but want to confirm that to ensure we don't give any surprises to our WPF friends.

jkoritzinsky · 2021-01-19T19:54:10Z

I've validated that this PR passes all of the C++/CLI tests that the C++ team runs.

jkotas

#46238 (comment)

…tub-cleanup

…l instead of the entry point and call TraceCall from all JIT_ReversePInvokeEnter* helpers.

Addressed linked comment.

jkoritzinsky · 2021-01-20T08:00:36Z

Merged in 5ec0c7a

GitHub messed up and didn’t mark the PR as merged. I’ll close the PR.

* Implement emitting an unmanaged calling convention entry point with the correct argument order and register usage on x86. * Move Unix x86 to the UnmanagedCallersOnly plan now that we don't need to do argument shuffling. * Add SEH hookup and profiler/debugger hooks to Reverse P/Invoke entry helper to match custom x86 thunk. Fixes dotnet#46177 * Remove Windows x86 assembly stub for individual reverse p/invokes. Move Windows x86 unmanaged callers only to not have extra overhead and put reverse P/Invoke stubs for Windows x86 on the UnmanagedCallersOnly plan. * Further cleanup * Remove extraneous UnmanagedCallersOnly block now that x86 UnmanagedCallersOnly has been simplified. * Undo ArgOrder size specifier since it isn't needed and it doesn't work. * Fix copy constructor reverse marshalling. Now that we don't have the emitted unmanaged thunk stub, we need to handle the x86 differences for copy-constructed parameters in the IL stub. * Fix version guid syntax. * Remove FastNExportHandler. * Revert "Remove FastNExportHandler." This reverts commit 423f70e. * Fix setting up entry frame for new thread. * Allow the NExportSEH record to live below ESP so we don't need to create a new stack frame. * Fix formatting. * Assign an offset for the return buffer on x86 since it might come in on the stack. * Make sure we use the TC block we just put in on x86 as well. * Shrink the ReversePInvokeFrame on non-x86 back to master's size. * Fix arch-specific R2R constant. * Pass the return address of the ReversePInvokeEnter helper to TraceCall instead of the entry point and call TraceCall from all JIT_ReversePInvokeEnter* helpers. * Fix ILVerification and ILVerify * fix R2R constants for crossgen1 * Don't assert ReversePInvokeFrame size for cross-bitness scenarios.

jkotas · 2021-01-20T12:07:41Z

src/coreclr/vm/jithelpers.cpp

+// As a result, we specially decorate this method to have the correct calling convention
+// and argument ordering for an HCALL, but we don't use the HCALL macros and contracts
+// since this method doesn't follow the contracts.
+void F_CALL_CONV HCCALL3(JIT_ReversePInvokeEnter, ReversePInvokeFrame* frame, CORINFO_METHOD_HANDLE handle, void* secretArg)


You have not addressed my comment about additional performance overhead of passing the extra method handle and secret arg arguments. It is introducing performance regression, it is breaking change for R2R (the helper has different signature now), and this design does not work for NativeAOT.

We should be paying this overhead only when somebody turns on the specific profiling notifications (they is rare and slow by design):

Add a flag to JIT/EE interface that controls whether

Keep the existing helper signature as is, introduce a new helper that takes these arguments

Call the new helper from JIT/EE interface only when the new flag is introduced

Could you please fix that?

Opened #47223

I will take a look at this ASAP.

jkotas · 2021-01-20T12:14:47Z

src/coreclr/vm/jithelpers.cpp

+{
+    _ASSERTE(frame != NULL && handle != NULL);
+
+    void* traceAddr = _ReturnAddress();


We should not need to fetch the return address upfront. It is only needed on the slow path when we are calling the rare helper.

jkoritzinsky added 7 commits December 16, 2020 10:49

Implement emitting an unmanaged calling convention entry point with t…

150d62e

…he correct argument order and register usage on x86.

Move Unix x86 to the UnmanagedCallersOnly plan now that we don't need…

c6f5880

… to do argument shuffling.

Add SEH hookup and profiler/debugger hooks to Reverse P/Invoke entry …

d54420b

…helper to match custom x86 thunk. Fixes dotnet#46177

Remove Windows x86 assembly stub for individual reverse p/invokes. Mo…

104a45a

…ve Windows x86 unmanaged callers only to not have extra overhead and put reverse P/Invoke stubs for Windows x86 on the UnmanagedCallersOnly plan.

Merge branch 'x86-reverse-stub-cleanup' of github.com:jkoritzinsky/ru…

8c56352

…ntime; branch 'master' of github.com:dotnet/runtime into x86-reverse-stub-cleanup

Further cleanup

927edc1

Remove extraneous UnmanagedCallersOnly block now that x86 UnmanagedCa…

a7236fd

…llersOnly has been simplified.

jkoritzinsky added the area-Interop-coreclr label Dec 19, 2020

jkoritzinsky added this to the 6.0.0 milestone Dec 19, 2020

jkoritzinsky requested review from jkotas and AaronRobinsonMSFT December 19, 2020 00:24

jkoritzinsky commented Dec 19, 2020

View reviewed changes

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

Undo ArgOrder size specifier since it isn't needed and it doesn't work.

4b99408

am11 reviewed Dec 19, 2020

View reviewed changes

src/coreclr/inc/jiteeversionguid.h Outdated Show resolved Hide resolved

jkotas reviewed Dec 19, 2020

View reviewed changes

src/coreclr/vm/jithelpers.cpp Show resolved Hide resolved

jkoritzinsky added 2 commits December 21, 2020 11:51

Fix copy constructor reverse marshalling. Now that we don't have the …

37c1753

…emitted unmanaged thunk stub, we need to handle the x86 differences for copy-constructed parameters in the IL stub.

Fix version guid syntax.

41d704e

jkoritzinsky added 6 commits December 21, 2020 14:10

Remove FastNExportHandler.

423f70e

Revert "Remove FastNExportHandler."

265fec6

This reverts commit 423f70e.

Fix setting up entry frame for new thread.

5745e72

Allow the NExportSEH record to live below ESP so we don't need to cre…

e5b97fb

…ate a new stack frame.

Fix formatting.

50712d5

Assign an offset for the return buffer on x86 since it might come in …

69e14c6

…on the stack.

janvorli reviewed Dec 28, 2020

View reviewed changes

src/coreclr/vm/dllimportcallback.cpp Show resolved Hide resolved

danmoseley mentioned this pull request Jan 3, 2021

Test_EventSource_EtwManifestGenerationRollover failed in CI #46518

Closed

AaronRobinsonMSFT mentioned this pull request Jan 6, 2021

[Regression] Invalid program when marhsalling delegate with both value type return and parameter #45637

Closed

jkoritzinsky added 3 commits January 6, 2021 15:51

Merge branch 'master' into x86-reverse-stub-cleanup

5564218

Make sure we use the TC block we just put in on x86 as well.

5f7a910

Merge branch 'master' into x86-reverse-stub-cleanup

b827ed1

jkoritzinsky mentioned this pull request Jan 12, 2021

Implement System.Runtime.InteropServices.CLong/CULong/NFloat #46401

Merged

sandreenko mentioned this pull request Jan 15, 2021

x86: support Return SIMD8. #46899

Merged

davmason mentioned this pull request Jan 15, 2021

Reverse PInvokes do not call TraceCall on non-x86 platforms #47037

Closed

sandreenko reviewed Jan 15, 2021

View reviewed changes

src/coreclr/jit/flowgraph.cpp Show resolved Hide resolved

sandreenko approved these changes Jan 15, 2021

View reviewed changes

jkotas reviewed Jan 15, 2021

View reviewed changes

src/coreclr/inc/readytorun.h Outdated Show resolved Hide resolved

Shrink the ReversePInvokeFrame on non-x86 back to master's size.

428a1fa

AaronRobinsonMSFT approved these changes Jan 19, 2021

View reviewed changes

jkotas previously requested changes Jan 19, 2021

View reviewed changes

jkoritzinsky added 5 commits January 19, 2021 15:49

Merge branch 'master' of github.com:dotnet/runtime into x86-reverse-s…

73729eb

…tub-cleanup

Fix arch-specific R2R constant.

07aee67

Pass the return address of the ReversePInvokeEnter helper to TraceCal…

22baef7

…l instead of the entry point and call TraceCall from all JIT_ReversePInvokeEnter* helpers.

Fix ILVerification and ILVerify

8ab33dc

fix R2R constants for crossgen1

7b621ab

Don't assert ReversePInvokeFrame size for cross-bitness scenarios.

9c7e6ac

jkoritzinsky closed this Jan 20, 2021

jkotas reviewed Jan 20, 2021

View reviewed changes

jkotas mentioned this pull request Jan 20, 2021

UnmanagedCallersOnly performance regression and R2R breaking change in #47223

Closed

jkotas reviewed Jan 20, 2021

View reviewed changes

jkoritzinsky deleted the x86-reverse-stub-cleanup branch January 21, 2021 19:29

jkotas mentioned this pull request Mar 2, 2021

Test failure: readytorun/coreroot_determinism/coreroot_determinism/coreroot_determinism.sh #45326

Closed

ghost locked as resolved and limited conversation to collaborators Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove extra UnmanagedCallersOnly overhead on x86 #46238

Remove extra UnmanagedCallersOnly overhead on x86 #46238

jkoritzinsky commented Dec 19, 2020 •

edited

Loading

jkotas commented Dec 19, 2020

jkoritzinsky commented Dec 21, 2020

jkoritzinsky commented Dec 24, 2020

sandreenko left a comment

AaronRobinsonMSFT left a comment

jkoritzinsky commented Jan 19, 2021

jkotas left a comment

jkoritzinsky commented Jan 20, 2021

jkotas Jan 20, 2021

jkotas Jan 20, 2021

jkoritzinsky Jan 20, 2021

jkotas Jan 20, 2021 •

edited

Loading

Remove extra UnmanagedCallersOnly overhead on x86 #46238

Remove extra UnmanagedCallersOnly overhead on x86 #46238

Conversation

jkoritzinsky commented Dec 19, 2020 • edited Loading

jkotas commented Dec 19, 2020

jkoritzinsky commented Dec 21, 2020

jkoritzinsky commented Dec 24, 2020

sandreenko left a comment

Choose a reason for hiding this comment

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

jkoritzinsky commented Jan 19, 2021

jkotas left a comment

Choose a reason for hiding this comment

jkoritzinsky commented Jan 20, 2021

jkotas Jan 20, 2021

Choose a reason for hiding this comment

jkotas Jan 20, 2021

Choose a reason for hiding this comment

jkoritzinsky Jan 20, 2021

Choose a reason for hiding this comment

jkotas Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

jkoritzinsky commented Dec 19, 2020 •

edited

Loading

jkotas Jan 20, 2021 •

edited

Loading