diff --git a/compiler/crates/react_compiler_inference/src/infer_mutation_aliasing_effects.rs b/compiler/crates/react_compiler_inference/src/infer_mutation_aliasing_effects.rs index 83b06418738..3c0bbedd386 100644 --- a/compiler/crates/react_compiler_inference/src/infer_mutation_aliasing_effects.rs +++ b/compiler/crates/react_compiler_inference/src/infer_mutation_aliasing_effects.rs @@ -11,7 +11,7 @@ //! creation, aliasing, mutation, freezing, and error conditions for each //! instruction and terminal in the HIR. -use indexmap::{IndexMap, IndexSet}; +use indexmap::IndexMap; use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; use react_compiler_diagnostics::CompilerDiagnostic; @@ -69,7 +69,7 @@ pub fn infer_mutation_aliasing_effects( value_id, AbstractValue { kind: ValueKind::Context, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }, ); initial_state.define(ctx_place.identifier, value_id); @@ -78,12 +78,12 @@ pub fn infer_mutation_aliasing_effects( let param_kind: AbstractValue = if is_function_expression { AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), } } else { AbstractValue { kind: ValueKind::Frozen, - reason: hashset_of(ValueReason::ReactiveFunctionArgument), + reason: ValueReasonSet::single(ValueReason::ReactiveFunctionArgument), } }; @@ -103,7 +103,7 @@ pub fn infer_mutation_aliasing_effects( value_id, AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }, ); initial_state.define(ref_place.identifier, value_id); @@ -185,7 +185,7 @@ pub fn infer_mutation_aliasing_effects( }; states_by_block.insert(block_id, incoming_state.clone()); - let mut state = incoming_state.clone(); + let mut state = incoming_state; infer_block(&mut context, &mut state, block_id, func, env)?; @@ -258,16 +258,88 @@ impl ValueId { // AbstractValue // ============================================================================= -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Copy)] struct AbstractValue { kind: ValueKind, - reason: IndexSet, + reason: ValueReasonSet, +} + +/// Capacity of [`ValueReasonSet`]. A set holds at most one of each `ValueReason` +/// variant, of which there are currently 12; the extra slots are headroom so +/// that adding variants upstream cannot overflow the set. +const VALUE_REASON_CAPACITY: usize = 16; + +/// An insertion-ordered set of [`ValueReason`]s, stored inline. +/// +/// This is a deliberate replacement for `IndexSet`, enabling insertion-order +/// memory while avoiding any heap allocation. At `AbstractValue`'s scale, this +/// has a dramatic impact on heap memory and wall time. +/// This takes advantage of the format of the data it's actually storing. A set +/// can hold at most one of each variant, so the members fit into a fixed inline +/// array. `ValueReason` is implemented as a single byte, so this struct is +/// ~18 bytes on the stack. +/// +/// Insertion order is preserved deliberately: [`primary_reason`] returns the +/// first non-`Other` member, matching the iteration order of the `Set` used by +/// the TypeScript implementation this is ported from. +#[derive(Debug, Clone, Copy)] +struct ValueReasonSet { + /// Members in insertion order. Only the first `len` entries are meaningful. + members: [ValueReason; VALUE_REASON_CAPACITY], + len: u8, } -fn hashset_of(r: ValueReason) -> IndexSet { - let mut s = IndexSet::default(); - s.insert(r); - s +impl Default for ValueReasonSet { + fn default() -> Self { + ValueReasonSet { + members: [ValueReason::Other; VALUE_REASON_CAPACITY], + len: 0, + } + } +} + +impl ValueReasonSet { + fn single(reason: ValueReason) -> Self { + let mut set = Self::default(); + set.insert(reason); + set + } + + fn contains(&self, reason: ValueReason) -> bool { + self.members[..self.len as usize].contains(&reason) + } + + fn iter(&self) -> impl Iterator + '_ { + self.members[..self.len as usize].iter().copied() + } + + /// Appends `reason` if not already present, preserving insertion order. + fn insert(&mut self, reason: ValueReason) { + if self.contains(reason) { + return; + } + debug_assert!( + (self.len as usize) < VALUE_REASON_CAPACITY, + "ValueReasonSet capacity must cover every ValueReason variant" + ); + if (self.len as usize) < VALUE_REASON_CAPACITY { + self.members[self.len as usize] = reason; + self.len += 1; + } + } + + /// True when every member of `other` is also a member of `self`. + fn is_superset_of(&self, other: &ValueReasonSet) -> bool { + other.iter().all(|reason| self.contains(reason)) + } + + /// Adds every member of `other`, keeping `self`'s existing order and + /// appending newcomers in `other`'s order — matching `IndexSet::insert`. + fn union_with(&mut self, other: &ValueReasonSet) { + for reason in other.iter() { + self.insert(reason); + } + } } // ============================================================================= @@ -315,7 +387,7 @@ impl InferenceState { } return AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }; } }; @@ -332,7 +404,7 @@ impl InferenceState { } merged_kind.unwrap_or_else(|| AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }) } @@ -360,7 +432,7 @@ impl InferenceState { vid, AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }, ); } @@ -438,7 +510,7 @@ impl InferenceState { value_id, AbstractValue { kind: ValueKind::Frozen, - reason: hashset_of(reason), + reason: ValueReasonSet::single(reason), }, ); // Note: In TS, this also transitively freezes FunctionExpression captures @@ -493,7 +565,7 @@ impl InferenceState { if let Some(other_value) = other.values.get(id) { let merged = merge_abstract_values(this_value, other_value); if merged.kind != this_value.kind - || !is_superset(&this_value.reason, &merged.reason) + || !this_value.reason.is_superset_of(&merged.reason) { let nv = next_values.get_or_insert_with(|| self.values.clone()); nv.insert(*id, merged); @@ -566,13 +638,6 @@ impl InferenceState { } } -fn is_superset( - a: &IndexSet, - b: &IndexSet, -) -> bool { - b.iter().all(|x| a.contains(x)) -} - #[derive(Debug, Clone, Copy)] enum MutateVariant { Mutate, @@ -738,13 +803,11 @@ fn hash_effect(effect: &AliasingEffect) -> String { fn merge_abstract_values(a: &AbstractValue, b: &AbstractValue) -> AbstractValue { let kind = merge_value_kinds(a.kind, b.kind); - if kind == a.kind && kind == b.kind && is_superset(&a.reason, &b.reason) { + if kind == a.kind && kind == b.kind && a.reason.is_superset_of(&b.reason) { return a.clone(); } - let mut reason = a.reason.clone(); - for r in &b.reason { - reason.insert(*r); - } + let mut reason = a.reason; + reason.union_with(&b.reason); AbstractValue { kind, reason } } @@ -1233,7 +1296,7 @@ fn apply_signature( vid, AbstractValue { kind: ValueKind::Mutable, - reason: hashset_of(ValueReason::Other), + reason: ValueReasonSet::single(ValueReason::Other), }, ); state.define(instr.lvalue.identifier, vid); @@ -1341,7 +1404,7 @@ fn apply_effect( value_id, AbstractValue { kind, - reason: hashset_of(reason), + reason: ValueReasonSet::single(reason), }, ); state.define(into.identifier, value_id); @@ -1370,7 +1433,7 @@ fn apply_effect( value_id, AbstractValue { kind: from_value.kind, - reason: from_value.reason.clone(), + reason: from_value.reason, }, ); state.define(into.identifier, value_id); @@ -1487,7 +1550,7 @@ fn apply_effect( } else { ValueKind::Frozen }, - reason: IndexSet::default(), + reason: ValueReasonSet::default(), }, ); state.define(into.identifier, value_id); @@ -1599,7 +1662,7 @@ fn apply_effect( value_id, AbstractValue { kind: from_value.kind, - reason: from_value.reason.clone(), + reason: from_value.reason, }, ); state.define(into.identifier, value_id); @@ -1615,7 +1678,7 @@ fn apply_effect( value_id, AbstractValue { kind: from_value.kind, - reason: from_value.reason.clone(), + reason: from_value.reason, }, ); state.define(into.identifier, value_id); @@ -3410,8 +3473,8 @@ fn compute_effects_for_aliasing_signature( /// since the primary reason is always inserted first, this effectively /// picks the most specific non-Other reason. We replicate this by /// preferring any non-Other reason over Other. -fn primary_reason(reasons: &IndexSet) -> ValueReason { - for &r in reasons { +fn primary_reason(reasons: &ValueReasonSet) -> ValueReason { + for r in reasons.iter() { if r != ValueReason::Other { return r; } @@ -3420,32 +3483,32 @@ fn primary_reason(reasons: &IndexSet) -> ValueReason } fn get_write_error_reason(abstract_value: &AbstractValue) -> String { - if abstract_value.reason.contains(&ValueReason::Global) { + if abstract_value.reason.contains(ValueReason::Global) { "Modifying a variable defined outside a component or hook is not allowed. Consider using an effect".to_string() - } else if abstract_value.reason.contains(&ValueReason::JsxCaptured) { + } else if abstract_value.reason.contains(ValueReason::JsxCaptured) { "Modifying a value used previously in JSX is not allowed. Consider moving the modification before the JSX".to_string() - } else if abstract_value.reason.contains(&ValueReason::Context) { + } else if abstract_value.reason.contains(ValueReason::Context) { "Modifying a value returned from 'useContext()' is not allowed.".to_string() } else if abstract_value .reason - .contains(&ValueReason::KnownReturnSignature) + .contains(ValueReason::KnownReturnSignature) { "Modifying a value returned from a function whose return value should not be mutated" .to_string() } else if abstract_value .reason - .contains(&ValueReason::ReactiveFunctionArgument) + .contains(ValueReason::ReactiveFunctionArgument) { "Modifying component props or hook arguments is not allowed. Consider using a local variable instead".to_string() - } else if abstract_value.reason.contains(&ValueReason::State) { + } else if abstract_value.reason.contains(ValueReason::State) { "Modifying a value returned from 'useState()', which should not be modified directly. Use the setter function to update instead".to_string() - } else if abstract_value.reason.contains(&ValueReason::ReducerState) { + } else if abstract_value.reason.contains(ValueReason::ReducerState) { "Modifying a value returned from 'useReducer()', which should not be modified directly. Use the dispatch function to update instead".to_string() - } else if abstract_value.reason.contains(&ValueReason::Effect) { + } else if abstract_value.reason.contains(ValueReason::Effect) { "Modifying a value used previously in an effect function or as an effect dependency is not allowed. Consider moving the modification before calling useEffect()".to_string() - } else if abstract_value.reason.contains(&ValueReason::HookCaptured) { + } else if abstract_value.reason.contains(ValueReason::HookCaptured) { "Modifying a value previously passed as an argument to a hook is not allowed. Consider moving the modification before calling the hook".to_string() - } else if abstract_value.reason.contains(&ValueReason::HookReturn) { + } else if abstract_value.reason.contains(ValueReason::HookReturn) { "Modifying a value returned from a hook is not allowed. Consider moving the modification into the hook where the value is constructed".to_string() } else { "This modifies a variable that React considers immutable".to_string() diff --git a/compiler/crates/react_compiler_inference/src/propagate_scope_dependencies_hir.rs b/compiler/crates/react_compiler_inference/src/propagate_scope_dependencies_hir.rs index d0538aad52f..2590a32125e 100644 --- a/compiler/crates/react_compiler_inference/src/propagate_scope_dependencies_hir.rs +++ b/compiler/crates/react_compiler_inference/src/propagate_scope_dependencies_hir.rs @@ -1472,32 +1472,36 @@ fn recursively_propagate_non_null( } // Compute intersection of 'done' neighbors only (filter out 'active' = cycle nodes) - let done_neighbor_sets: Vec> = neighbors - .iter() - .filter(|n| traversal_state.get(n) == Some(&TraversalState::Done)) - .filter_map(|n| working.get(n).cloned()) - .collect(); + let neighbor_intersection = { + let done_neighbor_sets: Vec<&BTreeSet> = neighbors + .iter() + .filter(|n| traversal_state.get(n) == Some(&TraversalState::Done)) + .filter_map(|n| working.get(n)) + .collect(); - let neighbor_intersection = if done_neighbor_sets.is_empty() { - BTreeSet::new() - } else { - let mut iter = done_neighbor_sets.into_iter(); - let first = iter.next().unwrap(); - iter.fold(first, |acc, s| acc.intersection(&s).copied().collect()) + match done_neighbor_sets.split_first() { + None => BTreeSet::new(), + Some((first, rest)) => rest.iter().fold((*first).clone(), |acc, s| { + acc.intersection(s).copied().collect() + }), + } }; - let prev_objects = working.get(&node_id).cloned().unwrap_or_default(); + // Temporarily remove the previous set out of the map so it can be safely + // borrowed and compared without a heavy deep clone. + let prev_objects = working.remove(&node_id).unwrap_or_default(); let mut merged: BTreeSet = prev_objects .union(&neighbor_intersection) .copied() .collect(); reduce_maybe_optional_chains(&mut merged, registry); - working.insert(node_id, merged.clone()); - traversal_state.insert(node_id, TraversalState::Done); - // Compare with previous value — can't just check size due to reduce_maybe_optional_chains changed |= prev_objects != merged; + + working.insert(node_id, merged); + traversal_state.insert(node_id, TraversalState::Done); + changed } diff --git a/compiler/crates/react_compiler_reactive_scopes/src/codegen_reactive_function.rs b/compiler/crates/react_compiler_reactive_scopes/src/codegen_reactive_function.rs index 9a722b105a0..c9063c5bd1a 100644 --- a/compiler/crates/react_compiler_reactive_scopes/src/codegen_reactive_function.rs +++ b/compiler/crates/react_compiler_reactive_scopes/src/codegen_reactive_function.rs @@ -555,14 +555,121 @@ pub fn codegen_function( // Context // ============================================================================= -type Temporaries = FxHashMap>; - #[derive(Clone)] enum ExpressionOrJsxText { Expression(Expression), JsxText(JSXText), } +/// The entry a write to [`Temporaries`] displaced, kept so the write can be +/// undone. +/// +/// The expression is boxed because `ExpressionOrJsxText` is ~900 bytes (it +/// inlines an `Expression`). Unboxed, the undo log would be a `Vec` of +/// ~900-byte slots that are almost always `Absent`, costing more peak heap than +/// the copy it replaces on shallow functions. Boxed, an entry is 16 bytes and +/// only allocates when a write actually displaces a buffered expression. +enum Displaced { + /// The key was not present before the write. + Absent, + /// The key was present as a declared temporary with no buffered value. + Empty, + /// The key was present with this buffered value. + Value(Box), +} + +/// A position in a [`Temporaries`] undo log, produced by [`Temporaries::mark`]. +#[derive(Clone, Copy)] +struct TempMark(usize); + +/// Expressions buffered for temporaries that have not been emitted yet, plus an +/// undo log allowing a nested block or scope to be codegen'd and its additions +/// discarded. +/// +/// The TypeScript implementation snapshots this with `new Map(cx.temp)`, which +/// is a *shallow* copy: it duplicates references, not the AST nodes behind them. +/// The equivalent Rust `.clone()` deep-copies every buffered `Expression` tree, +/// which made codegen quadratic in component size and dominated both allocation +/// volume and peak heap. +/// +/// `TS CodegenReactiveFunction.codegenBlock` asserts that pre-existing entries +/// are never mutated ("Expected temporary value to be unchanged"), so a +/// snapshot's only job is to discard entries added by the nested block. Because +/// entries are only ever inserted (never removed, nor mutated in place), +/// rewinding an insert log restores the map exactly, with no copying. +/// +/// All writes go through [`Temporaries::set`] so the log cannot drift out of +/// sync with the map. +#[derive(Default)] +struct Temporaries { + values: FxHashMap>, + journal: Vec<(DeclarationId, Displaced)>, +} + +impl Temporaries { + fn get(&self, declaration_id: DeclarationId) -> Option<&Option> { + self.values.get(&declaration_id) + } + + fn contains_key(&self, declaration_id: DeclarationId) -> bool { + self.values.contains_key(&declaration_id) + } + + /// Buffers `value` for `declaration_id`, journaling the displaced entry. + /// `HashMap::insert` returns that entry by move, so journaling costs no + /// clones. + fn set(&mut self, declaration_id: DeclarationId, value: Option) { + let displaced = match self.values.insert(declaration_id, value) { + None => Displaced::Absent, + Some(None) => Displaced::Empty, + Some(Some(previous)) => Displaced::Value(Box::new(previous)), + }; + self.journal.push((declaration_id, displaced)); + } + + /// Marks the current state, for a later [`Temporaries::rewind`]. + fn mark(&self) -> TempMark { + TempMark(self.journal.len()) + } + + /// Restores the state captured by `mark`, discarding every write since. + fn rewind(&mut self, mark: TempMark) { + while self.journal.len() > mark.0 { + let (declaration_id, displaced) = self.journal.pop().unwrap(); + match displaced { + Displaced::Absent => { + self.values.remove(&declaration_id); + } + Displaced::Empty => { + self.values.insert(declaration_id, None); + } + Displaced::Value(previous) => { + self.values.insert(declaration_id, Some(*previous)); + } + } + } + } + + /// Hands the buffered expressions to a nested function's context, which may + /// read them but must not leak its own additions back out. + /// + /// The borrower gets a fresh log, so [`Temporaries::reclaim`] can undo + /// exactly the borrower's writes rather than the lender's whole history. + fn lend(&mut self) -> Temporaries { + Temporaries { + values: std::mem::take(&mut self.values), + journal: Vec::new(), + } + } + + /// Takes back expressions handed out by [`Temporaries::lend`], discarding + /// every write the borrower made. + fn reclaim(&mut self, mut lent: Temporaries) { + lent.rewind(TempMark(0)); + self.values = lent.values; + } +} + struct Context<'env> { env: &'env mut Environment, #[allow(dead_code)] @@ -594,7 +701,7 @@ impl<'env> Context<'env> { fn_name, next_cache_index: 0, declarations: FxHashSet::default(), - temp: FxHashMap::default(), + temp: Temporaries::default(), object_methods: FxHashMap::default(), unique_identifiers, fbt_operands, @@ -653,8 +760,8 @@ fn codegen_reactive_function( ParamPattern::Place(p) => p, ParamPattern::Spread(sp) => &sp.place, }; - let ident = &cx.env.identifiers[place.identifier.0 as usize]; - cx.temp.insert(ident.declaration_id, None); + let declaration_id = cx.env.identifiers[place.identifier.0 as usize].declaration_id; + cx.temp.set(declaration_id, None); cx.declare(place.identifier); } @@ -732,9 +839,9 @@ fn convert_parameter( // ============================================================================= fn codegen_block(cx: &mut Context, block: &ReactiveBlock) -> Result { - let temp_snapshot: Temporaries = cx.temp.clone(); + let mark = cx.temp.mark(); let result = codegen_block_no_reset(cx, block)?; - cx.temp = temp_snapshot; + cx.temp.rewind(mark); Ok(result) } @@ -758,9 +865,9 @@ fn codegen_block_no_reset( scope, instructions, }) => { - let temp_snapshot = cx.temp.clone(); + let mark = cx.temp.mark(); codegen_reactive_scope(cx, &mut statements, *scope, instructions)?; - cx.temp = temp_snapshot; + cx.temp.rewind(mark); } ReactiveStatement::Terminal(term_stmt) => { let stmt = codegen_terminal(cx, &term_stmt.terminal)?; @@ -1258,8 +1365,9 @@ fn codegen_terminal( } => { let catch_param = match handler_binding.as_ref() { Some(binding) => { - let ident = &cx.env.identifiers[binding.identifier.0 as usize]; - cx.temp.insert(ident.declaration_id, None); + let declaration_id = + cx.env.identifiers[binding.identifier.0 as usize].declaration_id; + cx.temp.set(declaration_id, None); Some(PatternLike::Identifier(convert_identifier( binding.identifier, cx.env, @@ -1724,8 +1832,10 @@ fn codegen_store_or_declare( // Register temporaries for unnamed pattern operands for place in react_compiler_hir::visitors::each_pattern_operand(&lvalue.pattern) { let ident = &cx.env.identifiers[place.identifier.0 as usize]; - if kind != InstructionKind::Reassign && ident.name.is_none() { - cx.temp.insert(ident.declaration_id, None); + let declaration_id = ident.declaration_id; + let is_unnamed = ident.name.is_none(); + if kind != InstructionKind::Reassign && is_unnamed { + cx.temp.set(declaration_id, None); } } let rhs = codegen_place_to_expression(cx, val)?; @@ -1838,11 +1948,10 @@ fn emit_store( ReactiveValue::Instruction(InstructionValue::StoreContext { .. }) ); if !is_store_context { - let ident = &cx.env.identifiers[lvalue_place.identifier.0 as usize]; - cx.temp.insert( - ident.declaration_id, - Some(ExpressionOrJsxText::Expression(expr)), - ); + let declaration_id = + cx.env.identifiers[lvalue_place.identifier.0 as usize].declaration_id; + cx.temp + .set(declaration_id, Some(ExpressionOrJsxText::Expression(expr))); return Ok(None); } else { let stmt = @@ -1886,9 +1995,10 @@ fn codegen_instruction( })); }; let ident = &cx.env.identifiers[lvalue.identifier.0 as usize]; + let declaration_id = ident.declaration_id; if ident.name.is_none() { // temporary - cx.temp.insert(ident.declaration_id, Some(value)); + cx.temp.set(declaration_id, Some(value)); return Ok(Statement::EmptyStatement(EmptyStatement { base: BaseNode::typed("EmptyStatement"), })); @@ -2663,9 +2773,16 @@ fn codegen_function_expression( cx.unique_identifiers.clone(), cx.fbt_operands.clone(), ); - inner_cx.temp = cx.temp.clone(); + // The inner function reads the enclosing temporaries but must not leak its + // own back out. Lend the map to `inner_cx` and rewind its writes on the way + // out, rather than deep-cloning every buffered expression tree. The map is + // restored on the error path too, so `cx` is never left empty. + inner_cx.temp = cx.temp.lend(); - let fn_result = codegen_reactive_function(&mut inner_cx, &reactive_fn_mut)?; + let fn_result = codegen_reactive_function(&mut inner_cx, &reactive_fn_mut); + + cx.temp.reclaim(std::mem::take(&mut inner_cx.temp)); + let fn_result = fn_result?; let value = match expr_type { FunctionExpressionType::ArrowFunctionExpression => { @@ -2798,9 +2915,12 @@ fn codegen_object_expression( cx.unique_identifiers.clone(), cx.fbt_operands.clone(), ); - inner_cx.temp = cx.temp.clone(); + inner_cx.temp = cx.temp.lend(); + + let fn_result = codegen_reactive_function(&mut inner_cx, &reactive_fn_mut); - let fn_result = codegen_reactive_function(&mut inner_cx, &reactive_fn_mut)?; + cx.temp.reclaim(std::mem::take(&mut inner_cx.temp)); + let fn_result = fn_result?; ast_properties.push(ast_expr::ObjectExpressionProperty::ObjectMethod( ast_expr::ObjectMethod { @@ -3309,14 +3429,14 @@ fn codegen_place_to_expression( fn codegen_place(cx: &mut Context, place: &Place) -> Result { let ident = &cx.env.identifiers[place.identifier.0 as usize]; - if let Some(tmp) = cx.temp.get(&ident.declaration_id) { + if let Some(tmp) = cx.temp.get(ident.declaration_id) { if let Some(val) = tmp { return Ok(val.clone()); } // tmp is None — means declared but no temp value, fall through } // Check if it's an unnamed identifier without a temp - if ident.name.is_none() && !cx.temp.contains_key(&ident.declaration_id) { + if ident.name.is_none() && !cx.temp.contains_key(ident.declaration_id) { return Err(invariant_err( &format!( "[Codegen] No value found for temporary, identifier id={}", diff --git a/compiler/crates/react_compiler_validation/src/validate_preserved_manual_memoization.rs b/compiler/crates/react_compiler_validation/src/validate_preserved_manual_memoization.rs index b757a927dd9..e209441d3b9 100644 --- a/compiler/crates/react_compiler_validation/src/validate_preserved_manual_memoization.rs +++ b/compiler/crates/react_compiler_validation/src/validate_preserved_manual_memoization.rs @@ -142,18 +142,19 @@ fn visit_scope(scope_block: &ReactiveScopeBlock, state: &mut VisitorState) { if let Some(ref memo_state) = state.manual_memo_state { if let Some(ref deps_from_source) = memo_state.deps_from_source { let scope = &state.env.scopes[scope_block.scope.0 as usize]; + // `dependencies` still has to be cloned because `env` is passed + // mutably below. `temporaries`, `decls` and `deps_from_source` do + // not: they live in fields disjoint from `env`, so they can simply + // be borrowed. let deps = scope.dependencies.clone(); let memo_loc = memo_state.loc; - let decls = memo_state.decls.clone(); - let deps_from_source = deps_from_source.clone(); - let temporaries = state.temporaries.clone(); for dep in &deps { validate_inferred_dep( dep.identifier, &dep.path, - &temporaries, - &decls, - &deps_from_source, + &state.temporaries, + &memo_state.decls, + deps_from_source, state.env, memo_loc, ); diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js index 6be61ffc2ca..efd69d0dc5e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzStaticBrowser-test.js @@ -1584,6 +1584,32 @@ describe('ReactDOMFizzStaticBrowser', () => { expect(lifetimes[0].aborted).toBe(true); }); + it('constructs no abort controller when no signal is passed', async () => { + // The render lifetime exists only to bound the caller's abort listener, + // so a render that is given no signal has nothing to bound and should not + // pay for a controller. It also means such a render never requires + // AbortController to exist or to work. + const RealAbortController = globalThis.AbortController; + let constructed = 0; + globalThis.AbortController = class extends RealAbortController { + constructor() { + super(); + constructed++; + } + }; + + try { + const stream = await serverAct(() => + ReactDOMFizzServer.renderToReadableStream(
hello world
), + ); + await readContent(stream); + } finally { + globalThis.AbortController = RealAbortController; + } + + expect(constructed).toBe(0); + }); + it('attaches no listener when the signal is already aborted', async () => { const controller = new AbortController(); controller.abort(); diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index a863863ea60..1f249556b94 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -435,7 +435,9 @@ export opaque type Request = { onFatalError: (error: mixed) => void, // Aborted once the render ends, whether it completed, failed fatally or was // aborted. Bounds the lifetime of anything that must not outlive the render. - renderLifetimeController: AbortController, + // Null until attachAbortSignal creates it, so a render that is given no + // signal constructs no controller. + renderLifetimeController: null | AbortController, // Form state that was the result of an MPA submission, if it was provided. formState: null | ReactFormState, // DEV-only, warning dedupe @@ -589,7 +591,7 @@ function RequestInstance( this.onShellReady = onShellReady === undefined ? noop : onShellReady; this.onShellError = onShellError === undefined ? noop : onShellError; this.onFatalError = onFatalError === undefined ? noop : onFatalError; - this.renderLifetimeController = new AbortController(); + this.renderLifetimeController = null; this.formState = formState === undefined ? null : formState; if (__DEV__) { this.didWarnForKey = null; @@ -1455,7 +1457,7 @@ function fatalError( } onFatalError(error); } - request.renderLifetimeController.abort(RENDER_ENDED); + endRenderLifetime(request); if (request.destination !== null) { request.status = CLOSED; closeWithError(request.destination, error); @@ -6352,7 +6354,7 @@ function flushCompletedQueues( } } // We're done. - request.renderLifetimeController.abort(RENDER_ENDED); + endRenderLifetime(request); request.status = CLOSED; close(destination); // We need to stop flowing now because we do not want any async contexts which might call @@ -6516,6 +6518,13 @@ function finishAbort(request: Request, abortableTasks: Set): void { } } +function endRenderLifetime(request: Request): void { + const renderLifetimeController = request.renderLifetimeController; + if (renderLifetimeController !== null) { + renderLifetimeController.abort(RENDER_ENDED); + } +} + // Aborts the request when the caller's signal aborts. The render lifetime // bounds the listener, so the runtime removes the listener as soon as the // render ends. From that point on abort() returns early, so the listener has @@ -6533,12 +6542,14 @@ export function attachAbortSignal(request: Request, signal: AbortSignal): void { abort(request, signal.reason); return; } + const renderLifetimeController = new AbortController(); + request.renderLifetimeController = renderLifetimeController; signal.addEventListener( 'abort', () => { abort(request, signal.reason); }, - {signal: request.renderLifetimeController.signal}, + {signal: renderLifetimeController.signal}, ); } @@ -6552,7 +6563,7 @@ export function abort(request: Request, reason: mixed): void { // can be aborted. in practice this makes abort callable at most once per render. return; } - request.renderLifetimeController.abort(RENDER_ENDED); + endRenderLifetime(request); const isRecoverableReason = typeof reason === 'object' && reason !== null &&