Josh Groves (Jan 26 2024 at 15:37): Josh Groves (Jan 26 2024 at 15:37):Hi! I was just looking at a performance profile and noticed function calls to clone during drop at https://github.com/bytecodealliance/wasmtime/commit/0977952dcd9d482bff7c288868ccb52769b3a92e#diff-e95cf87da8a8dcc08101777a3375ca3e634f0ed6b9f7c5daeec7e73cc4610eafR1078
It looks like it was added for a security advisory a while ago. It seems like it's bumping a refcount for the module, so I was wondering how it helps with the note in the advisory or if it might be redundant (e.g., the Arc seems like it's already guaranteed to be held through the duration of drop, unless something depends on the actual refcount somewhere)
Alex Crichton (Jan 26 2024 at 17:42):This specific one may no longer be required, I'd have to dig around a bit more, but in general an instantiation of a module needs to keep a strong reference to the module in case the module is dropped elsewhere because the underlying code can't go away. There's other mechanisms we have for preserving this though and this may be redundant, I sort of forget.
Would you happen to have a benchmark to poke at to help investigate the issues here? (microbenchmark is fine)
Josh Groves (Jan 26 2024 at 18:08):I don't have a great benchmark to share right now but it's probably not a real-world performance problem. I just happened to see a clone being called from a drop in a performance profile flame chart and was curious about it (I think the sampling profiler just happened to sample it a few times by chance)
Alex Crichton (Jan 26 2024 at 18:35):oh! I misinterpreted this
Alex Crichton (Jan 26 2024 at 18:35):oh ok no that clone is "I don't want to write unsafe code here and it's easier to just clone"
Alex Crichton (Jan 26 2024 at 18:35):it is not fundamentally required since it's already rooted elsewhere
Josh Groves (Jan 26 2024 at 18:57):That makes sense, thank you!
Last updated: Dec 23 2024 at 12:05 UTC