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

[mono][aot] Optimize constrained calls made from gsharedvt methods. #79339

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Dec 7, 2022

The calls are of the form:
.constrained T_GSHAREDVT
callvirt

Whenever T_GSHAREDVT is a reference or value type is only known at runtime.

Previously these were handled by passing the arguments to a JIT icall which computed the target method and did a runtime invoke.

Add 2 optimizations:

  • Precompute the data which depends only on the type and the method, store it in an rgctx slot and pass it to the JIT icall.
  • Add a fastpath for simpler cases which makes an indirect call from generated code.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@vargaz vargaz removed the request for review from SamMonoRT December 7, 2022 10:46
@vargaz
Copy link
Contributor Author

vargaz commented Dec 7, 2022

This will also avoid some interpreter transitions on wasm, making the stack traces on BCL test suite crashes etc. smaller.

@vargaz
Copy link
Contributor Author

vargaz commented Dec 8, 2022

The failures look relevant.

@SamMonoRT
Copy link
Member

* do an indirect call.
*/
calls [0] = NULL;
// FIXME: Add more cases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what would be the additional cases that would fit into the fastpath approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cases excluded by the if.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand the code and the effect that covering more cases would bring.

@vargaz
Copy link
Contributor Author

vargaz commented Dec 12, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 13, 2022
@vargaz
Copy link
Contributor Author

vargaz commented Dec 13, 2022

Failures are relevant.

@vargaz
Copy link
Contributor Author

vargaz commented Dec 15, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 15, 2022
@vargaz
Copy link
Contributor Author

vargaz commented Dec 15, 2022

Failures are unrelated.

@lewing lewing closed this Dec 20, 2022
@lewing lewing reopened this Dec 20, 2022
@vargaz
Copy link
Contributor Author

vargaz commented Dec 20, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +2639 to +2660
/* Lookup the virtual method */
mono_class_setup_vtable (klass);
g_assert (m_class_get_vtable (klass));
vt_slot = mono_method_get_vtable_slot (cmethod);
if (mono_class_is_interface (cmethod->klass)) {
iface_offset = mono_class_interface_offset (klass, cmethod->klass);
g_assert (iface_offset != -1);
vt_slot += iface_offset;
}
m = m_class_get_vtable (klass) [vt_slot];
if (cmethod->is_inflated) {
m = mono_class_inflate_generic_method_full_checked (m, NULL, mono_method_get_context (cmethod), error);
return_val_if_nok (error, NULL);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have some helper function that does this already?

The calls are of the form:
.constrained T_GSHAREDVT
callvirt <method>

Whenever T_GSHAREDVT is a reference or value type is only known at runtime.

Previously these were handled by passing the arguments to a JIT icall which
computed the target method and did a runtime invoke.

Added 2 optimizations:
* Precompute the data which depends only on the type and the method,
  store it in an rgctx slot and pass it to the JIT icall.
* Add a fastpath for simpler cases which makes an indirect call
  from generated code.
@vargaz
Copy link
Contributor Author

vargaz commented Feb 7, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Feb 8, 2023

Failures are unrelated

@vargaz vargaz merged commit 1b788f4 into dotnet:main Feb 8, 2023
@vargaz vargaz deleted the constrained-gsharedvt-opt branch February 8, 2023 04:01
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2023
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.

7 participants