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] Don't use direct wrapper when invoking cctors, so all cctors can share one wrapper #102375

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kg
Copy link
Contributor

@kg kg commented May 17, 2024

Should improve interp startup time a bit, since right now we generate one wrapper for every cctor, and lots of cctors get called during app startup. From my testing this doesn't break AOT, but I can't tell whether this is accidentally causing us to interpret cctors that were AOT'd.

I added some new assertions in various places because of some crashes I hit while making my way to this change; happy to take them out.

cc @BrzVlad @kotlarmilos would appreciate thoughts on the right way to do this. this is just my first attempt that worked, i still don't understand all this code well

my thinking here:

  • the calli family of opcodes expect a function pointer, but it may not be possible to have a function pointer for all cctors (i.e. wasm where you can't jit arbitrary function pointers at runtime, at least not yet). and making a fn ptr for them is wasteful since we call them exactly once. so i added calli_monomethod, which expects a monomethod. interp_runtime_invoke is passing a monomethod to the wrapper, so this makes it compatible when it wasn't before.
  • only enabled for cctors right now to reduce the blast radius. From my testing cctors are the main semi-hot path where we rely on runtime invoke.
  • should interp_runtime_invoke actually be passing monomethod? it seems like that only works because direct wrappers ignore the method pointer, i can't figure out why it's passing one. (obviously, the new calli_monomethod uses it)
  • the new calli_monomethod has assertions to ensure we're only using it for the expected scenario
  • we previously assumed that the call target would always be native for a runtime_invoke wrapper, which isn't true at least in the wasm interp scenario since we can't jit a function pointer for it. so i added a check that sets native=0 if jit call isn't supported for the target. i'm guessing i got this wrong and it will need to change for jit and aot to work correctly.

…invoke, so we can share the wrapper for all cctors
@kg
Copy link
Contributor Author

kg commented May 17, 2024

Surprisingly, this passes tests. So it just needs expert review I think, especially to make sure it's not going to regress AOT startup by knocking cctors into the interp or perhaps demoting AOT'd wrappers to interpreted ones.

@BrzVlad
Copy link
Member

BrzVlad commented May 24, 2024

This approach looks fragile and confusing to me. I think we should just have a different wrapper for runtime invoke that we know can only be called from interpreter (and that should never be jit/aot compiled). It could completely reuse the code for the already existing runtime invoke wrappers.

I think we should be able to pass pretty much just the InterpMethod* as argument. Seems quite dubious that with each unique interp ldftn we would attempt to aot-lookup the method. I think this should be more lazy, but it needs some thinking.

@kg
Copy link
Contributor Author

kg commented May 24, 2024

This approach looks fragile and confusing to me. I think we should just have a different wrapper for runtime invoke that we know can only be called from interpreter (and that should never be jit/aot compiled). It could completely reuse the code for the already existing runtime invoke wrappers.

Should I add a new wrapper type or a new wrapper subtype? I think changing the generated code for the wrapper shouldn't be too hard.

I think we should be able to pass pretty much just the InterpMethod* as argument. Seems quite dubious that with each unique interp ldftn we would attempt to aot-lookup the method. I think this should be more lazy, but it needs some thinking.

OK, so it needs to be calli_interpmethod, not calli_monomethod? I can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants