Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move return buffer handling from interop to the JIT #39294
Move return buffer handling from interop to the JIT #39294
Changes from 73 commits
c17d786
8eecd35
c06a040
9448965
7a26d40
a75f746
4f09a4f
e4aaf91
3d66e53
4eee458
a7e0b52
8eb6846
110c2ae
96336f5
48e3fce
b83c60a
f3bdf58
e7b8369
5324b7e
3c60ca2
f23c291
0a6790b
925ec02
47553e5
779708a
4b8870b
6c56bc8
3897473
426b0f3
cc73336
a825b8a
9c0cbb1
63507b7
b72dcb4
1262c27
ef2adac
5c2a9bc
dd44e32
1056c44
a0218a1
47bf6aa
a1bb21c
254063b
71adacb
195713b
8833b06
7e2c1f9
382113f
bd4022a
c4acc94
1694da8
5dd9031
7182ad0
e329242
94f6a1d
cfce1d7
0368407
a4286f3
bae4165
77d6bf6
9e965f8
c28925e
e5212cb
e020165
90cfcef
41b21a5
f6eeddf
71c9032
b334b37
28b1d48
aab5ca0
96081a0
0ef4948
6ee4a82
3c89d74
2ce4270
fb00bc9
8f9478a
68d4081
e26c36d
c1040aa
fa2f2aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I know you don't want to change the managed calling convention here but could you please clarify which decisions are temporary here. For example. on x64 we pass 8 bytes struct in a reg even if it has several fields, but we restrict x86 to 1 field, is it a temporary solution?
Allowing all structs <= pointer size to be passed as register would simplify the checks in this method and decrease the number of JIT-EE calls.
The same question about 8 byte struct, would they be passed as regs in managed calling conv in the future changes on x86?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these decisions can be changed in a future PR if we're willing to take the R2R compat break. To my knowledge, changing the calling convention breaks ready-to-run compatibility, so I didn't want to do that in this PR if I could avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of back and forth with the EE; would it make sense to do all this work over on the jit interface side?
(I'm guessing we rarely iterate in this loop, but still...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very unlikely we'll iterate through this loop more than once in my experience, but I can move this to the other side of the ee-jit interface if you think that's better for perf. I was trying to avoid the hassle of versioning the jit-ee interface, but I'm fine doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine if you just measured how often we have to iterate (over say PMI FX) and just confirmed it is truly rare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run the PMI FX diff as you recommended and confirmed that in the whole framework we run the loop body at most twice.
We run the loop body twice in 557 cases (551 cases are in Roslyn, the remaining 6 are in System.Reflection.Metadata), and we run the loop body only once in 102592 cases.
So, we run the loop body only once in 0.5% of cases if we include the Roslyn assemblies in Core_Root, or approximately 0.005% of cases if we exclude the Roslyn assemblies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we only make it past the first checks in the function (the pointer-sized check and the first "is value type with one field" check) 19% of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will soon forget what VM used to unwrap, maybe delete an outdated reference and say something like:
// Note that on Windows we only pass specific pointer-sized structs that satisfy
isTrivialPointerSizedStruct
checksThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the body says:
Is it still true? What does this method return for a trivial struct on x86 unix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The native calling conventions always uses a return buffer for structures, even non-trivial ones, on x86 unix. For the managed calling convention, we'll continue to support the current system (4-byte structs returned in registers).