Skip to content
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

Convert HELPER_METHOD_FRAME to QCalls (4/N) #95670

Merged
merged 11 commits into from
Dec 8, 2023
Merged

Convert HELPER_METHOD_FRAME to QCalls (4/N) #95670

merged 11 commits into from
Dec 8, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Dec 6, 2023

No description provided.

@hughbe
Copy link
Contributor

hughbe commented Dec 6, 2023

Jan - is there an issue/PR somewhere that explains the rationale behind these changes (perf, correctness, other?)? just curious!

@DragosDanielBoia
Copy link
Contributor

@dotnet-policy-service rerun

@am11
Copy link
Member

am11 commented Dec 6, 2023

@hughbe, perhaps there is no tracking issue, but I think this is a general goodness to have fewer FCalls as possible https://github.com/dotnet/runtime/blob/b345e2dbd6936eb281fadb7d70473358be578d24/docs/design/coreclr/botr/corelib.md#choosing-between-fcall-qcall-pinvoke-and-writing-in-managed-code. The ultimate goal is to gradually get rid of HELPER_METHOD_FRAME (dotnet/corert#3784 (comment)), and thereby reduce/remove the dependency on libunwind from Unix.

@jkotas, are we strictly converting FCalls to QCalls in this series, or to managed as well (when there is an opportunity)?

@jkotas
Copy link
Member Author

jkotas commented Dec 6, 2023

Jan - is there an issue/PR somewhere that explains the rationale behind these changes (perf, correctness, other?)?

Good point. I have opened #95695. It is something that myself, @AaronRobinsonMSFT, @davidwrighton discussed over lunch as a general goodness a few weeks ago. I have decided to work on it as my side-project.

@jkotas
Copy link
Member Author

jkotas commented Dec 6, 2023

@jkotas, are we strictly converting FCalls to QCalls in this series, or to managed as well (when there is an opportunity)?

Yes, if there is an easy opportunity. There are a few cases like that in this PR:

  • ArgIterator.GetRemainingCount - move to managed
  • StubHelpers - move a call of managed pointer to managed
  • Moved argument checking to managed in some places

@davidwrighton
Copy link
Member

This looks good although I might instead of converting the trailing byte code into a QCall instead produce a QCall/FCall that can produce the SyncBlock and then do the rest in managed. However, we could tackle that later.

@jkotas
Copy link
Member Author

jkotas commented Dec 7, 2023

I might instead of converting the trailing byte code into a QCall instead produce a QCall/FCall that can produce the SyncBlock and then do the rest in managed. However, we could tackle that later.

Yes, it is a trade-off in how many data type definition to mirror between managed and unmanaged sides. I have been leaning towards keeping this mirroring more minimal.

@jkotas jkotas merged commit 6dc1087 into dotnet:main Dec 8, 2023
178 of 180 checks passed
@jkotas jkotas deleted the hmf4 branch December 8, 2023 02:43
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants