Skip to content

Commit 1c788af

Browse files
fitzgenfitzgen
authored
Only const-eval passive element segments once (#12985)
* Only const-eval passive element segments once Not each time they are referenced by e.g. an `array.new_elem` instruction. * fix non-gc builds/warnings * update tests; remove redundant tests
1 parent b3df295 commit 1c788af

16 files changed

Lines changed: 466 additions & 487 deletions

File tree

‎crates/environ/src/compile/module_environ.rs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ use crate::module::{
33
FuncRefIndex, Initializer, MemoryInitialization, MemoryInitializer, Module, TableSegment,
44
TableSegmentElements,
55
};
6-
use crate::prelude::*;
76
use crate::{
87
ConstExpr, ConstOp, DataIndex, DefinedFuncIndex, ElemIndex, EngineOrModuleTypeIndex,
98
EntityIndex, EntityType, FuncIndex, FuncKey, GlobalIndex, IndexType, InitMemory, MemoryIndex,
@@ -12,6 +11,7 @@ use crate::{
1211
Tunables, TypeConvert, TypeIndex, WasmError, WasmHeapTopType, WasmHeapType, WasmResult,
1312
WasmValType, WasmparserTypeConverter,
1413
};
14+
use crate::{NeedsGcRooting, prelude::*};
1515
use cranelift_entity::SecondaryMap;
1616
use cranelift_entity::packed_option::ReservedValue;
1717
use std::borrow::Cow;
@@ -531,7 +531,8 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
531531
}
532532
TableSegmentElements::Functions(elems.into())
533533
}
534-
ElementItems::Expressions(_ty, items) => {
534+
ElementItems::Expressions(ty, items) => {
535+
let ty = self.convert_ref_type(ty)?;
535536
let mut exprs =
536537
Vec::with_capacity(usize::try_from(items.count()).unwrap());
537538
for expr in items {
@@ -541,7 +542,14 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
541542
self.flag_func_escaped(func);
542543
}
543544
}
544-
TableSegmentElements::Expressions(exprs.into())
545+
TableSegmentElements::Expressions {
546+
needs_gc_rooting: if ty.is_vmgcref_type_and_not_i31() {
547+
NeedsGcRooting::Yes
548+
} else {
549+
NeedsGcRooting::No
550+
},
551+
exprs: exprs.into(),
552+
}
545553
}
546554
};
547555

@@ -565,12 +573,12 @@ impl<'a, 'data> ModuleEnvironment<'a, 'data> {
565573

566574
ElementKind::Passive => {
567575
let elem_index = ElemIndex::from_u32(index as u32);
568-
let index = self.result.module.passive_elements.len();
569-
self.result.module.passive_elements.push(elements)?;
576+
let passive_index =
577+
self.result.module.passive_elements.push(elements)?;
570578
self.result
571579
.module
572580
.passive_elements_map
573-
.insert(elem_index, index);
581+
.insert(elem_index, passive_index);
574582
}
575583

576584
ElementKind::Declared => {}
@@ -1309,7 +1317,7 @@ impl ModuleTranslation<'_> {
13091317
// expressions are deferred to get evaluated at runtime.
13101318
let function_elements = match &segment.elements {
13111319
TableSegmentElements::Functions(indices) => indices,
1312-
TableSegmentElements::Expressions(_) => break,
1320+
TableSegmentElements::Expressions { .. } => break,
13131321
};
13141322

13151323
let precomputed =

‎crates/environ/src/module.rs‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,15 @@ pub struct TableSegment {
266266
pub elements: TableSegmentElements,
267267
}
268268

269+
/// Does something need GC rooting?
270+
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
271+
pub enum NeedsGcRooting {
272+
/// GC rooting is needed.
273+
Yes,
274+
/// GC rooting is not needed.
275+
No,
276+
}
277+
269278
/// Elements of a table segment, either a list of functions or list of arbitrary
270279
/// expressions.
271280
#[derive(Clone, Debug, Serialize, Deserialize)]
@@ -274,15 +283,21 @@ pub enum TableSegmentElements {
274283
/// indicates a null function.
275284
Functions(Box<[FuncIndex]>),
276285
/// Arbitrary expressions, aka either functions, null or a load of a global.
277-
Expressions(Box<[ConstExpr]>),
286+
Expressions {
287+
/// Is this expression's result of a type that needs GC rooting and
288+
/// tracing?
289+
needs_gc_rooting: NeedsGcRooting,
290+
/// The const expressions for this segment's elements.
291+
exprs: Box<[ConstExpr]>,
292+
},
278293
}
279294

280295
impl TableSegmentElements {
281296
/// Returns the number of elements in this segment.
282297
pub fn len(&self) -> u64 {
283298
match self {
284299
Self::Functions(s) => u64::try_from(s.len()).unwrap(),
285-
Self::Expressions(s) => u64::try_from(s.len()).unwrap(),
300+
Self::Expressions { exprs, .. } => u64::try_from(exprs.len()).unwrap(),
286301
}
287302
}
288303
}
@@ -316,10 +331,11 @@ pub struct Module {
316331
pub memory_initialization: MemoryInitialization,
317332

318333
/// WebAssembly passive elements.
319-
pub passive_elements: TryVec<TableSegmentElements>,
334+
pub passive_elements: TryPrimaryMap<PassiveElemIndex, TableSegmentElements>,
320335

321-
/// The map from passive element index (element segment index space) to index in `passive_elements`.
322-
pub passive_elements_map: BTreeMap<ElemIndex, usize>,
336+
/// The map from passive element index (element segment index space) to
337+
/// index in `passive_elements`.
338+
pub passive_elements_map: BTreeMap<ElemIndex, PassiveElemIndex>,
323339

324340
/// The map from passive data index (data segment index space) to index in `passive_data`.
325341
pub passive_data_map: BTreeMap<DataIndex, Range<u32>>,

‎crates/environ/src/types.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1692,11 +1692,19 @@ impl Default for VMSharedTypeIndex {
16921692
pub struct DataIndex(u32);
16931693
entity_impl_with_try_clone!(DataIndex);
16941694

1695-
/// Index type of a passive element segment inside the WebAssembly module.
1695+
/// Index type of an element segment inside the WebAssembly module.
16961696
#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Debug, Serialize, Deserialize)]
16971697
pub struct ElemIndex(u32);
16981698
entity_impl_with_try_clone!(ElemIndex);
16991699

1700+
/// Dense index space of the subset of element segments that are passive.
1701+
///
1702+
/// Not a spec-level concept, just used to get dense index spaces for passive
1703+
/// element segments inside of Wasmtime.
1704+
#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Debug, Serialize, Deserialize)]
1705+
pub struct PassiveElemIndex(u32);
1706+
entity_impl_with_try_clone!(PassiveElemIndex);
1707+
17001708
/// Index type of a defined tag inside the WebAssembly module.
17011709
#[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Debug, Serialize, Deserialize)]
17021710
pub struct DefinedTagIndex(u32);

‎crates/wasmtime/src/runtime/externals/table.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,10 @@ impl Table {
131131
///
132132
/// Panics if `store` does not own this table.
133133
pub fn ty(&self, store: impl AsContext) -> TableType {
134-
self._ty(store.as_context().0)
134+
self.ty_(store.as_context().0)
135135
}
136136

137-
fn _ty(&self, store: &StoreOpaque) -> TableType {
137+
pub(crate) fn ty_(&self, store: &StoreOpaque) -> TableType {
138138
TableType::from_wasmtime_table(store.engine(), self.wasmtime_ty(store))
139139
}
140140

@@ -185,7 +185,7 @@ impl Table {
185185
.ok()?
186186
.map(|r| r.unchecked_copy())
187187
.map(|r| store.clone_gc_ref(&r));
188-
Some(match self._ty(&store).element().heap_type().top() {
188+
Some(match self.ty_(&store).element().heap_type().top() {
189189
HeapType::Extern => {
190190
Ref::Extern(gc_ref.map(|r| ExternRef::from_cloned_gc_ref(&mut store, r)))
191191
}
@@ -220,7 +220,7 @@ impl Table {
220220
}
221221

222222
pub(crate) fn set_(&self, store: &mut StoreOpaque, index: u64, val: Ref) -> Result<()> {
223-
let ty = self._ty(store);
223+
let ty = self.ty_(store);
224224
match element_type(&ty) {
225225
TableElementType::Func => {
226226
let element = val.into_table_func(store, ty.element())?;
@@ -250,10 +250,10 @@ impl Table {
250250
///
251251
/// Panics if `store` does not own this table.
252252
pub fn size(&self, store: impl AsContext) -> u64 {
253-
self._size(store.as_context().0)
253+
self.size_(store.as_context().0)
254254
}
255255

256-
pub(crate) fn _size(&self, store: &StoreOpaque) -> u64 {
256+
pub(crate) fn size_(&self, store: &StoreOpaque) -> u64 {
257257
// unwrap here should be ok because the runtime should always guarantee
258258
// that we can fit the number of elements in a 64-bit integer.
259259
u64::try_from(store[self.instance].table(self.index).current_elements).unwrap()
@@ -503,7 +503,7 @@ impl Table {
503503
val: Ref,
504504
len: u64,
505505
) -> Result<()> {
506-
let ty = self._ty(&store);
506+
let ty = self.ty_(&store);
507507
match element_type(&ty) {
508508
TableElementType::Func => {
509509
let val = val.into_table_func(store, ty.element())?;
@@ -531,7 +531,7 @@ impl Table {
531531
#[cfg(feature = "gc")]
532532
pub(crate) fn trace_roots(&self, store: &mut StoreOpaque, gc_roots_list: &mut vm::GcRootsList) {
533533
if !self
534-
._ty(store)
534+
.ty_(store)
535535
.element()
536536
.is_vmgcref_type_and_points_to_object()
537537
{

‎crates/wasmtime/src/runtime/func.rs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1057,7 +1057,15 @@ impl Func {
10571057
/// This value is safe to pass to [`Func::from_raw`] so long as the same
10581058
/// `store` is provided.
10591059
pub fn to_raw(&self, mut store: impl AsContextMut) -> *mut c_void {
1060-
self.vm_func_ref(store.as_context_mut().0).as_ptr().cast()
1060+
self.to_raw_(store.as_context_mut().0)
1061+
}
1062+
1063+
pub(crate) fn to_raw_(&self, store: &mut StoreOpaque) -> *mut c_void {
1064+
self.vm_func_ref(store).as_ptr().cast()
1065+
}
1066+
1067+
pub(crate) fn to_val_raw(&self, store: &mut StoreOpaque) -> ValRaw {
1068+
ValRaw::funcref(self.to_raw_(store))
10611069
}
10621070

10631071
/// Invokes this function with the `params` given, returning the results

‎crates/wasmtime/src/runtime/gc/disabled/anyref.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@ impl AnyRef {
7575
match *self {}
7676
}
7777

78+
pub(crate) fn _to_raw(&self, _store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
79+
match *self {}
80+
}
81+
7882
pub fn ty(&self, _store: impl AsContext) -> Result<HeapType> {
7983
match *self {}
8084
}

‎crates/wasmtime/src/runtime/gc/disabled/externref.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,16 @@ impl ExternRef {
4343
None
4444
}
4545

46-
pub fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
46+
pub(crate) fn _from_raw(_store: &mut AutoAssertNoGc<'_>, raw: u32) -> Option<Rooted<Self>> {
4747
assert_eq!(raw, 0);
4848
None
4949
}
5050

5151
pub fn to_raw(&self, _store: impl AsContextMut) -> Result<u32> {
5252
match *self {}
5353
}
54+
55+
pub(crate) fn _to_raw(&self, _store: &mut AutoAssertNoGc<'_>) -> Result<u32> {
56+
match *self {}
57+
}
5458
}

‎crates/wasmtime/src/runtime/linker.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1404,7 +1404,7 @@ impl Definition {
14041404
*size = m.size();
14051405
}
14061406
Definition::Extern(Extern::Table(m), DefinitionType::Table(_, size)) => {
1407-
*size = m._size(store);
1407+
*size = m.size_(store);
14081408
}
14091409
_ => {}
14101410
}
@@ -1415,7 +1415,7 @@ impl DefinitionType {
14151415
pub(crate) fn from(store: &StoreOpaque, item: &Extern) -> DefinitionType {
14161416
match item {
14171417
Extern::Func(f) => DefinitionType::Func(f.type_index(store)),
1418-
Extern::Table(t) => DefinitionType::Table(*t.wasmtime_ty(store), t._size(store)),
1418+
Extern::Table(t) => DefinitionType::Table(*t.wasmtime_ty(store), t.size_(store)),
14191419
Extern::Global(t) => DefinitionType::Global(*t.wasmtime_ty(store)),
14201420
Extern::Memory(t) => {
14211421
DefinitionType::Memory(*t.wasmtime_ty(store), t.internal_size(store))

‎crates/wasmtime/src/runtime/store.rs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2109,18 +2109,30 @@ impl StoreOpaque {
21092109
if asyncness != Asyncness::No {
21102110
vm::Yield::new().await;
21112111
}
2112+
21122113
#[cfg(feature = "stack-switching")]
21132114
{
21142115
self.trace_wasm_continuation_roots(gc_roots_list);
21152116
if asyncness != Asyncness::No {
21162117
vm::Yield::new().await;
21172118
}
21182119
}
2120+
21192121
self.trace_vmctx_roots(gc_roots_list);
21202122
if asyncness != Asyncness::No {
21212123
vm::Yield::new().await;
21222124
}
2125+
2126+
self.trace_instance_roots(gc_roots_list);
2127+
if asyncness != Asyncness::No {
2128+
vm::Yield::new().await;
2129+
}
2130+
21232131
self.trace_user_roots(gc_roots_list);
2132+
if asyncness != Asyncness::No {
2133+
vm::Yield::new().await;
2134+
}
2135+
21242136
self.trace_pending_exception_roots(gc_roots_list);
21252137

21262138
log::trace!("End trace GC roots")
@@ -2250,6 +2262,19 @@ impl StoreOpaque {
22502262
log::trace!("End trace GC roots :: vmctx");
22512263
}
22522264

2265+
#[cfg(feature = "gc")]
2266+
fn trace_instance_roots(&mut self, gc_roots_list: &mut GcRootsList) {
2267+
log::trace!("Begin trace GC roots :: instance");
2268+
for (_id, instance) in &mut self.instances {
2269+
// SAFETY: the instance's GC roots will remain valid for the
2270+
// duration of this GC cycle.
2271+
unsafe {
2272+
instance.handle.get_mut().trace_roots(gc_roots_list);
2273+
}
2274+
}
2275+
log::trace!("End trace GC roots :: instance");
2276+
}
2277+
22532278
#[cfg(feature = "gc")]
22542279
fn trace_user_roots(&mut self, gc_roots_list: &mut GcRootsList) {
22552280
log::trace!("Begin trace GC roots :: user");

‎crates/wasmtime/src/runtime/values.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,12 @@ impl Val {
265265
/// The returned [`ValRaw`] does not carry type information and is only safe
266266
/// to use within the context of this store itself. For more information see
267267
/// [`ExternRef::to_raw`] and [`Func::to_raw`].
268-
pub fn to_raw(&self, store: impl AsContextMut) -> Result<ValRaw> {
268+
pub fn to_raw(&self, mut store: impl AsContextMut) -> Result<ValRaw> {
269+
let mut store = AutoAssertNoGc::new(store.as_context_mut().0);
270+
self.to_raw_(&mut store)
271+
}
272+
273+
pub(crate) fn to_raw_(&self, store: &mut AutoAssertNoGc) -> Result<ValRaw> {
269274
match self {
270275
Val::I32(i) => Ok(ValRaw::i32(*i)),
271276
Val::I64(i) => Ok(ValRaw::i64(*i)),
@@ -274,18 +279,18 @@ impl Val {
274279
Val::V128(b) => Ok(ValRaw::v128(b.as_u128())),
275280
Val::ExternRef(e) => Ok(ValRaw::externref(match e {
276281
None => 0,
277-
Some(e) => e.to_raw(store)?,
282+
Some(e) => e._to_raw(store)?,
278283
})),
279284
Val::AnyRef(e) => Ok(ValRaw::anyref(match e {
280285
None => 0,
281-
Some(e) => e.to_raw(store)?,
286+
Some(e) => e._to_raw(store)?,
282287
})),
283288
Val::ExnRef(e) => Ok(ValRaw::exnref(match e {
284289
None => 0,
285-
Some(e) => e.to_raw(store)?,
290+
Some(e) => e._to_raw(store)?,
286291
})),
287292
Val::FuncRef(f) => Ok(ValRaw::funcref(match f {
288-
Some(f) => f.to_raw(store),
293+
Some(f) => f.to_raw_(store),
289294
None => ptr::null_mut(),
290295
})),
291296
Val::ContRef(_) => {

0 commit comments

Comments
 (0)