[wasm] Notify jiterpreter when a method is freed #89980
Conversation
kg
added
NO-MERGE
ghost
added
the
needs-area-label
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsIn some cases a MonoMethod and its associated InterpMethod can be freed at runtime. The jiterpreter currently knows nothing about this, and in very rare circumstances it's possible that something new would get allocated at the same address. Very bad things would probably happen if so. This PR updates the jiterpreter to clear freed methods from its JIT queues and discards some state so that if a new object is allocated at the same address, it won't inherit old information. It also changes some tables that previously used raw memory addresses to use assigned indices instead, which should help with the 'we freed something and then put something new there' problem. It is very hard to manually reproduce the scenario that I think caused #89670 but I was at least able to verify during testing that methods can be freed while the jit queue is not empty, so hopefully this will fix that. At some point we probably also want to update the jiterpreter so that if a method is freed, we remove the function pointer so that the browser is able to potentially release the memory used by the compiled traces inside of it. But that's a big undertaking thanks to threads, so it's not in this PR. Depends on #88279
|
|
Updated so that the interp_entry and jit_call queues are now stored in the native heap in thread-local lists, and there's a central list tracking all of those lists. So any time a method is freed we can acquire a lock and go through all the queues and ensure the freed method isn't in them, synchronously. This avoids the need to somehow push a callback to all of our threads. (We do still need to clean up their state, but it's less important) |
Notify jiterpreter when a method is freed and ensure it isn't in the jit queues Fix infosByMethod not being populated Checkpoint: Migrate jiterpreter jit queues to C thread-local memory Use tlqueue for interp_entry as well
kg
requested review from
BrzVlad,
kotlarmilos,
lewing,
pavelsavara and
vargaz
as code owners
kg
added
area-Codegen-Interpreter-mono
and removed
NO-MERGE
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos Issue DetailsIn some cases a MonoMethod and its associated InterpMethod can be freed at runtime. The jiterpreter currently knows nothing about this, and in very rare circumstances it's possible that something new would get allocated at the same address. Very bad things would probably happen if so. This PR updates the jiterpreter to clear freed methods from its JIT queues and discards some state so that if a new object is allocated at the same address, it won't inherit old information. It also changes some tables that previously used raw memory addresses to use assigned indices instead, which should help with the 'we freed something and then put something new there' problem. It is very hard to manually reproduce the scenario that I think caused #89670 but I was at least able to verify during testing that methods can be freed while the jit queue is not empty, so hopefully this will fix that. At some point we probably also want to update the jiterpreter so that if a method is freed, we remove the function pointer so that the browser is able to potentially release the memory used by the compiled traces inside of it. But that's a big undertaking thanks to threads, so it's not in this PR. Depends on #88279
|
lambdageek
left a comment
lambdageek
left a comment
There was a problem hiding this comment.
LGTM.
Two areas to call out (but I'm not sure if there's anything to be done about it):
- the
EMSCRIPTEN_KEEPALIVEfunctions are pretty fragile since they don't have GC state transitions and use OS (not coop) mutexes. It might be enough to just comment that adding work inside those locks needs to be done carefully. - I don't know if the jiterp would ever run outside of WASM, but we do have mono helpers for the TLS keys and atomic increments.
|
Should we take this in main? Should it still be considered for 8? |
We would have to backport both this and multithreading support to take this in 8. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
In some cases a MonoMethod and its associated InterpMethod can be freed at runtime. The jiterpreter currently knows nothing about this, and in very rare circumstances it's possible that something new would get allocated at the same address. Very bad things would probably happen if so.
This PR updates the jiterpreter to clear freed methods from its JIT queues and discards some state so that if a new object is allocated at the same address, it won't inherit old information.
It also changes some tables that previously used raw memory addresses to use assigned indices instead, which should help with the 'we freed something and then put something new there' problem.
It is very hard to manually reproduce the scenario that I think caused #89670 but I was at least able to verify during testing that methods can be freed while the jit queue is not empty, so hopefully this will fix that.
At some point we probably also want to update the jiterpreter so that if a method is freed, we remove the function pointer so that the browser is able to potentially release the memory used by the compiled traces inside of it. But that's a big undertaking thanks to threads, so it's not in this PR.
Depends on #88279