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.

Sign up for GitHub

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

Jump to bottom

feat: lazily generate instanceFlags #275

Merged
merged 6 commits into from
Nov 27, 2023
Merged

feat: lazily generate instanceFlags #275

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f4ae669
style: use find_map() instead of filter_map().next()
bnjbvr Nov 24, 2023
53b2ff5
feat: only generate instance flags globals when they're used
bnjbvr Nov 24, 2023
d96e5e6
Merge branch 'main' into dce-instanceflags
guybedford Nov 24, 2023
0e7f6a1
Merge branch 'main' into dce-instanceflags
guybedford Nov 25, 2023
e151895
Merge branch 'main' into dce-instanceflags
bnjbvr Nov 27, 2023
ce4ef6a
review: rename ensure_instance_flags to instance_flags
bnjbvr Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions crates/js-component-bindgen/src/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,23 @@ impl Source {
}
}

pub fn prepend_str(&mut self, src: &str) {
// Infer the indent at start: it's the difference between the size of the first line, and
// its size if trimmed at the left. That raw difference is in number of spaces, not in
// units of indent; since each indent is 2 spaces, divide by 2 at the end.
let indent = self
.s
.lines()
.next()
.map_or(0, |line| (line.len() - line.trim_start().len()) / 2);
let mut new_start = Source {
s: String::new(),
indent,
};
new_start.push_str(src);
self.s = new_start.s + &self.s;
}

pub fn indent(&mut self, amt: usize) {
self.indent += amt;
}
Expand Down
41 changes: 29 additions & 12 deletions crates/js-component-bindgen/src/transpile_bindgen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ use crate::{uwrite, uwriteln};
use base64::{engine::general_purpose, Engine as _};
use heck::*;
use indexmap::IndexMap;
use std::cell::RefCell;
use std::collections::{BTreeMap, BTreeSet, HashMap};
use std::fmt::Write;
use std::mem;
use wasmtime_environ::component::{
ComponentTypes, InterfaceType, TypeDef, TypeFuncIndex, TypeResourceTableIndex,
ComponentTypes, InterfaceType, RuntimeComponentInstanceIndex, TypeDef, TypeFuncIndex,
TypeResourceTableIndex,
};
use wasmtime_environ::{
component,
Expand Down Expand Up @@ -141,6 +143,7 @@ pub fn transpile_bindgen(
lowering_options: Default::default(),
imports_resource_map: Default::default(),
exports_resource_map: Default::default(),
used_instance_flags: Default::default(),
defined_resource_classes: Default::default(),
resource_dtors: Default::default(),
resource_tables_initialized: (0..component.component.num_resource_tables)
Expand All @@ -152,6 +155,7 @@ pub fn transpile_bindgen(
instantiator.instantiate();
instantiator.ensure_resource_tables();
instantiator.destructors();
instantiator.ensure_instance_flags();
instantiator.gen.src.js(&instantiator.src.js);
instantiator.gen.src.js_init(&instantiator.src.js_init);

Expand Down Expand Up @@ -353,6 +357,8 @@ struct Instantiator<'a, 'b> {
imports: BTreeMap<String, WorldKey>,
imports_resource_map: ResourceMap,
exports_resource_map: ResourceMap,
/// Instance flags which references have been emitted externally at least once.
used_instance_flags: RefCell<BTreeSet<RuntimeComponentInstanceIndex>>,
defined_resource_classes: BTreeSet<String>,
resource_dtors: BTreeMap<TypeId, CoreDef>,
lowering_options:
Expand Down Expand Up @@ -475,10 +481,6 @@ impl<'a> Instantiator<'a, '_> {
}

fn instantiate(&mut self) {
for i in 0..self.component.num_runtime_component_instances {
uwriteln!(self.src.js_init, "const instanceFlags{i} = new WebAssembly.Global({{ value: \"i32\", mutable: true }}, {});", wasmtime_environ::component::FLAG_MAY_LEAVE | wasmtime_environ::component::FLAG_MAY_ENTER);
}

for (i, trampoline) in self.translation.trampolines.iter() {
let Trampoline::LowerImport {
index,
Expand Down Expand Up @@ -549,11 +551,10 @@ impl<'a> Instantiator<'a, '_> {
.component
.initializers
.iter()
.filter_map(|i| match i {
.find_map(|i| match i {
GlobalInitializer::Resource(r) if r.index == resource_idx => Some(r),
_ => None,
})
.next()
.unwrap();

if let Some(dtor) = &resource_def.dtor {
Expand Down Expand Up @@ -599,6 +600,20 @@ impl<'a> Instantiator<'a, '_> {
}
}

fn ensure_instance_flags(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

// SAFETY: short-lived borrow, and the refcell isn't mutably borrowed in the loop's body.
let mut instance_flag_defs = String::new();
for used in self.used_instance_flags.borrow().iter() {
let i = used.as_u32();
uwriteln!(
&mut instance_flag_defs,
"const instanceFlags{i} = new WebAssembly.Global({{ value: \"i32\", mutable: true }}, {});",
wasmtime_environ::component::FLAG_MAY_LEAVE
| wasmtime_environ::component::FLAG_MAY_ENTER);
}
self.src.js_init.prepend_str(&instance_flag_defs);
}

fn destructors(&mut self) {
for (ty, dtor) in self.resource_dtors.iter() {
let dtor_name_str = self.core_def(dtor);
Expand Down Expand Up @@ -707,11 +722,10 @@ impl<'a> Instantiator<'a, '_> {
.component
.initializers
.iter()
.filter_map(|i| match i {
.find_map(|i| match i {
GlobalInitializer::Resource(r) if r.index == resource_idx => Some(r),
_ => None,
})
.next()
.unwrap();

if let Some(dtor) = &resource_def.dtor {
Expand Down Expand Up @@ -1054,11 +1068,10 @@ impl<'a> Instantiator<'a, '_> {
.component
.initializers
.iter()
.filter_map(|i| match i {
.find_map(|i| match i {
GlobalInitializer::Resource(r) if r.index == resource_idx => Some(r),
_ => None,
})
.next()
.unwrap();

if let Some(dtor) = &resource_def.dtor {
Expand Down Expand Up @@ -1435,7 +1448,11 @@ impl<'a> Instantiator<'a, '_> {
match def {
CoreDef::Export(e) => self.core_export(e),
CoreDef::Trampoline(i) => format!("trampoline{}", i.as_u32()),
CoreDef::InstanceFlags(i) => format!("instanceFlags{}", i.as_u32()),
CoreDef::InstanceFlags(i) => {
// SAFETY: short-lived borrow-mut.
self.used_instance_flags.borrow_mut().insert(*i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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_def take &mut sef: didn't work with existing code where this would cause mutable vs immutable conflicts
  • pass a instance_flags: &mut BTreeSet parameter
    • if that variable lived in self and 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.

Hence the inner mutability. Do you have a preference over how to achieve this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh I see, makes sense, thanks for explaining the reasoning here!

format!("instanceFlags{}", i.as_u32())
}
}
}

Expand Down