feat: lazily generate instanceFlags
#275
Conversation
bnjbvr
force-pushed
the
dce-instanceflags
branch
from
923f158
to
53b2ff5
Compare
|
Got hit by the temporal dead zone, aka use-before-def of a let binding. So I had to add a |
| @@ -599,6 +600,20 @@ impl<'a> Instantiator<'a, '_> { | |||
| } | |||
| } | |||
|
|
|||
| fn ensure_instance_flags(&mut self) { | |||
There was a problem hiding this comment.
We can just call this instance_flags I think, since the ensure naming is usually for cases where we need to check it against an already-written list.
| CoreDef::InstanceFlags(i) => format!("instanceFlags{}", i.as_u32()), | ||
| CoreDef::InstanceFlags(i) => { | ||
| // SAFETY: short-lived borrow-mut. | ||
| self.used_instance_flags.borrow_mut().insert(*i); |
There was a problem hiding this comment.
These are quite a few hoops just to deal with a u32. I'd suggest rather just cloning it into the set directly to avoid all the other borrow complexity. (it's underlying datatype is just a u32)
There was a problem hiding this comment.
oh it's because I'm mutating the used_instance_flags that I need the inner mutability with RefCell. The alternatives that I've discarded were the following:
- have
core_deftake&mut sef: didn't work with existing code where this would cause mutable vs immutable conflicts - pass a
instance_flags: &mut BTreeSetparameter- if that variable lived in
selfand is mutably borrowed at call sites, we get back to the previous bullet item - if not, then I found it a bit invasive to pass it down to all the callers, from the top-level.
- if that variable lived in
Hence the inner mutability. Do you have a preference over how to achieve this?
There was a problem hiding this comment.
Ahh I see, makes sense, thanks for explaining the reasoning here!
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
|
Thanks for the review! Is the merge queue buggy? It seems the checks have passed, and it claims it's waiting for other status checks? |
|
Yeah the merge queue is buggy, merged manually. It might be worth disabling merge queue, it's mostly been trouble so far... |
This lazily generates the
instanceFlags, instead of emitting them all at once. The rationale is that if they're not required (e.g. in use cases involving only leaf components, if I understand correctly), then we can avoid allocating them and we can remove a dependency on the WebAssembly object being present. A future change could be to not use WebAssembly globals to represent the instance flags, but instead to use some global JS state, eliminating the dependency on the WebAssembly object more generally.