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 all FCalls with HELPER_METHOD_FRAME to QCalls #95695

Open
jkotas opened this issue Dec 6, 2023 · 9 comments · Fixed by #96916
Open

Convert all FCalls with HELPER_METHOD_FRAME to QCalls #95695

jkotas opened this issue Dec 6, 2023 · 9 comments · Fixed by #96916

Comments

@jkotas
Copy link
Member

jkotas commented Dec 6, 2023

Benefits:

  • Performance: QCalls are generally faster than FCalls with HELPER_METHOD_FRAME
  • Eliminate duplication: These are redundant mechanism. Once all of them are converted, we can eliminate the duplication.
  • Complexity of stackwalking algorithm: HELPER_METHOD_FRAMEs make the stackwalking algorithm with all its dependencies more complicated than it needs to be. (See nibblemapmacros do a linear scan #93550 for more context.)
    • We want to work towards unification of CoreCLR stackwalker and native AOT stackwalker. Native AOT has much simpler stackwalker and it does not have HELPER_METHOD_FRAMEs.
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 6, 2023
@jkotas jkotas added area-VM-coreclr and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 6, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Dec 6, 2023
@hughbe
Copy link
Contributor

hughbe commented Dec 6, 2023

This is cool! Is the plan to eventually remove FCall functionality from the runtime?

@danmoseley
Copy link
Member

Good to see you around here again @hughbe! 😀

@jkotas
Copy link
Member Author

jkotas commented Dec 6, 2023

Is the plan to eventually remove FCall functionality from the runtime?

FCalls with HELPER_METHOD_FRAME. I think FCalls without HELPER_METHOD_FRAME are needed for perf reasons.

@davidwrighton
Copy link
Member

I think we also might be able to move to QCalls with SuppressGCTransition instead of FCalls without HELPER_METHOD_FRAME for the perf critical ones, but that's a bridge to examine and consider crossing once we've removed HELPER_METHOD_FRAME from the situation.

@jkotas
Copy link
Member Author

jkotas commented Dec 7, 2023

I think we also might be able to move to QCalls with SuppressGCTransition instead of FCalls without HELPER_METHOD_FRAME for the perf critical ones, but that's a bridge to examine and consider crossing once we've removed HELPER_METHOD_FRAME from the situation.

I agree. It would require modifying rules for how GC tracked pointers are passed to and from P/Invokes and we would need to deal with managed/unmanaged calling convention differences somehow. I think it would be just a cosmetic change at the end. It would not be a significant simplification of the system (compared to getting rid of HELPER_METHOD_FRAMEs).

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 12, 2024
jkotas added a commit to jkotas/runtime that referenced this issue Jan 12, 2024
FCThrow is implemented usign HELPER_METHOD_FRAME. All
uses of FCThrow need to be removed to fix dotnet#95695.
jkotas added a commit that referenced this issue Jan 13, 2024
FCThrow is implemented usign HELPER_METHOD_FRAME. All uses of FCThrow need to be removed to fix #95695.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 13, 2024
tmds pushed a commit to tmds/runtime that referenced this issue Jan 23, 2024
FCThrow is implemented usign HELPER_METHOD_FRAME. All uses of FCThrow need to be removed to fix dotnet#95695.
@huoyaoyuan
Copy link
Member

I believe this was closed unintentionally 😄

In addition, I'd like to see some documentation about the more and more unmanaged-like unsafe code in CoreLib. I'm still learning and trying case by case.

@jkotas jkotas reopened this Jan 26, 2024
@jkotas
Copy link
Member Author

jkotas commented Jan 26, 2024

I'd like to see some documentation about the more and more unmanaged-like unsafe code in CoreLib.

What would you like to see? The documentation about QCalls and related topics is in https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/corelib.md .

@huoyaoyuan
Copy link
Member

The existing documentation is about "manual managed" code. Nowadays there are more "manual unmanaged" code in CoreLib, for example, dealing with MethodTable and manipulating GC refs as bytes.

@jkotas
Copy link
Member Author

jkotas commented Jan 29, 2024

Feel free to propose documentation update in a PR.

The examples do not look unique to CoreLib. MethodTable is an example of an unmanaged resource with lifetime associated with managed object. These situations occur in interop quite frequently. GC.KeepAlive documentation has an example with COM.

AustinWise added a commit to AustinWise/runtime that referenced this issue Mar 16, 2024
This has a similar structure to FailFast as converted in dotnet#98908.

Contributes to dotnet#95695
jkotas added a commit that referenced this issue Mar 17, 2024
* Convert GetCurrentMethod to QCALL

This has a similar structure to FailFast as converted in #98908.

Contributes to #95695

* Simplify code for QCALL

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants