Skip to content

react-compiler: fix memo blocks around an expression that reads a local and then assigns it #42940

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

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

react-compiler: fix memo blocks around an expression that reads a local and then assigns it #42940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
152 changes: 152 additions & 0 deletions src/react_compiler/inference/propagate_scope_dependencies_hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ fn find_temporaries_used_outside_declaring_scope(
let mut pruned_scopes: HashSet<ScopeId> = HashSet::default();
let mut traversal = ScopeBlockTraversal::new();
let mut used_outside_declaring_scope: HashSet<DeclarationId> = HashSet::default();
let mut reassigned_locals = LoadsOfReassignedLocals::default();

let handle_place = |place_id: IdentifierId,
declarations: &IdMap<DeclarationId, ScopeId>,
Expand Down Expand Up @@ -182,6 +183,12 @@ fn find_temporaries_used_outside_declaring_scope(
env,
);
}
reassigned_locals.visit_instruction(
instr,
func,
env,
&mut used_outside_declaring_scope,
);
// Handle instruction (track declarations)
let current_scope = traversal.current_scope();
if let Some(scope) = current_scope {
Expand Down Expand Up @@ -215,11 +222,156 @@ fn find_temporaries_used_outside_declaring_scope(
env,
);
}
reassigned_locals.visit_terminal(&block.terminal, env, &mut used_outside_declaring_scope);
}

used_outside_declaring_scope
}

/// Not in upstream: finds a temporary that is read after the local it was loaded from is reassigned.
#[derive(Default)]
struct LoadsOfReassignedLocals {
/// A local that some instruction reassigns, with the id of its latest reassignment so far.
reassigned_at: IdMap<DeclarationId, EvaluationOrder>,
/// A temporary loaded from such a local or from a property path on it: the local and the id of the load.
loads: IdMap<DeclarationId, (DeclarationId, EvaluationOrder)>,
}

impl LoadsOfReassignedLocals {
fn visit_use(&self, place: &Place, stale: &mut HashSet<DeclarationId>, env: &Environment) {
let temporary = env.identifiers[place.identifier.0 as usize].declaration_id;
let Some(&(local, loaded_at)) = self.loads.get(temporary) else {
return;
};
if self
.reassigned_at
.get(local)
.is_some_and(|&reassigned_at| reassigned_at > loaded_at)
{
stale.insert(temporary);
}
}

fn visit_terminal(
&self,
terminal: &Terminal,
env: &Environment,
stale: &mut HashSet<DeclarationId>,
) {
if self.loads.is_empty() {
return;
}
match terminal {
// A case test is lowered ahead of the discriminant and prints behind it, where it runs.
Terminal::Switch { test, .. } => self.visit_use(test, stale, env),
_ => {
for operand in visitors::each_terminal_operand(terminal) {
self.visit_use(&operand, stale, env);
}
}
}
}

fn visit_instruction(
&mut self,
instr: &Instruction,
func: &HirFunction,
env: &Environment,
stale: &mut HashSet<DeclarationId>,
) {
if !self.loads.is_empty() {
// Codegen prints the property of a method call through the receiver, which is checked.
let method = match &instr.value {
InstructionValue::MethodCall { property, .. } => Some(property.identifier),
_ => None,
};
for operand in visitors::each_instruction_operand(instr, env) {
if Some(operand.identifier) != method {
self.visit_use(&operand, stale, env);
}
}
}
match &instr.value {
InstructionValue::DeclareLocal { lvalue, .. }
| InstructionValue::StoreLocal { lvalue, .. } => {
self.visit_store(lvalue.kind, &lvalue.place, instr.id, func, env);
}
InstructionValue::Destructure { lvalue, .. }
if matches!(
lvalue.kind,
InstructionKind::Let | InstructionKind::Reassign
) =>
{
for place in visitors::each_pattern_operand(&lvalue.pattern) {
self.visit_store(lvalue.kind, &place, instr.id, func, env);
}
}
InstructionValue::PrefixUpdate { lvalue, .. }
| InstructionValue::PostfixUpdate { lvalue, .. } => {
self.visit_store(InstructionKind::Reassign, lvalue, instr.id, func, env);
}
_ => self.visit_load(instr, env),
}
}

fn visit_store(
&mut self,
kind: InstructionKind,
place: &Place,
id: EvaluationOrder,
func: &HirFunction,
env: &Environment,
) {
let local = env.identifiers[place.identifier.0 as usize].declaration_id;
match kind {
// rewrite_instruction_kinds_based_on_reassignment.rs keeps `Let` for a local that is reassigned.
InstructionKind::Let => {
self.reassigned_at
.entry(local)
.or_insert(EvaluationOrder(0));
}
InstructionKind::Reassign => {
// No `Let` declaration comes before this, as for a parameter: collect the loads so far.
if self.reassigned_at.insert(local, id).is_none() {
self.visit_loads_before(id, func, env);
}
}
_ => {}
}
}

fn visit_loads_before(&mut self, id: EvaluationOrder, func: &HirFunction, env: &Environment) {
for (_block_id, block) in &func.body.blocks {
for &instr_id in &block.instructions {
let instr = &func.instructions[instr_id.0 as usize];
if instr.id >= id {
return;
}
self.visit_load(instr, env);
}
}
}

fn visit_load(&mut self, instr: &Instruction, env: &Environment) {
let declaration_id =
|place: &Place| env.identifiers[place.identifier.0 as usize].declaration_id;
let local = match &instr.value {
InstructionValue::LoadLocal { place, .. } => {
Some(declaration_id(place)).filter(|local| self.reassigned_at.contains_key(*local))
}
InstructionValue::PropertyLoad { object, .. } => self
.loads
.get(declaration_id(object))
.map(|&(local, _)| local),
_ => None,
};
if let Some(local) = local {
self.loads
.insert(declaration_id(&instr.lvalue), (local, instr.id));
}
}
}

// =============================================================================
// collectTemporariesSidemap
// =============================================================================
Expand Down
36 changes: 29 additions & 7 deletions src/react_compiler/reactive_scopes/promote_used_temporaries.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,8 @@ enum Effect {
None,
Read,
Write,
/// An assignment to a local. It also conflicts with a memo block that starts after it: a cache hit skips the block.
Store,
}

#[derive(Clone, Copy)]
Expand All @@ -581,6 +583,8 @@ struct InterState<'a> {
statements: Statements,
/// Statements that print in place inside the value block being visited. They count only there.
value_block_statements: Option<Statements>,
/// Position of the entry of the innermost memo block being visited.
memo_block: u32,
/// fbt wants these inside the macro call: they stay inline and their operands get the names.
inline_macro_operands: &'a HashSet<IdentifierId>,
operands_of_macro_operands: IdMap<DeclarationId, Vec<Place>>,
Expand All @@ -593,6 +597,7 @@ impl<'a> InterState<'a> {
position: 0,
statements: Statements::default(),
value_block_statements: None,
memo_block: 0,
inline_macro_operands,
operands_of_macro_operands: IdMap::new(),
}
Expand All @@ -618,7 +623,7 @@ impl<'a> InterState<'a> {
match effect {
Effect::None => {}
Effect::Read => statements.read = self.position,
Effect::Write => statements.write = self.position,
Effect::Write | Effect::Store => statements.write = self.position,
}
}

Expand All @@ -627,7 +632,9 @@ impl<'a> InterState<'a> {
match temporary.effect {
Effect::None => {}
Effect::Read => statements.read = statements.read.max(temporary.position),
Effect::Write => statements.write = statements.write.max(temporary.position),
Effect::Write | Effect::Store => {
statements.write = statements.write.max(temporary.position)
}
}
}

Expand All @@ -639,9 +646,15 @@ impl<'a> InterState<'a> {
Effect::None => false,
Effect::Read => write > temporary.position,
Effect::Write => read.max(write) > temporary.position,
Effect::Store => read.max(write).max(self.memo_block) > temporary.position,
}
}

fn enter_memo_block(&mut self) -> u32 {
self.position += 1;
std::mem::replace(&mut self.memo_block, self.position)
}

fn enter_value_block(&mut self) -> Option<Statements> {
let outer = self.value_block_statements;
self.value_block_statements = Some(outer.unwrap_or_default());
Expand Down Expand Up @@ -675,6 +688,7 @@ fn promote_interposed_block(
}) {
inter_state.statement(Effect::Read);
}
let outer = inter_state.enter_memo_block();
promote_interposed_block(
&scope.instructions,
state,
Expand All @@ -683,6 +697,7 @@ fn promote_interposed_block(
globals,
env,
);
inter_state.memo_block = outer;
}
ReactiveStatement::PrunedScope(scope) => {
promote_interposed_block(
Expand Down Expand Up @@ -807,16 +822,23 @@ fn instruction_effect(
| InstructionValue::PropertyDelete { .. }
| InstructionValue::ComputedStore { .. }
| InstructionValue::ComputedDelete { .. }
| InstructionValue::PostfixUpdate { .. }
| InstructionValue::PrefixUpdate { .. }
| InstructionValue::StoreGlobal { .. }
| InstructionValue::StoreGlobal { .. } => Effect::Write,
InstructionValue::PostfixUpdate { .. } | InstructionValue::PrefixUpdate { .. } => {
Effect::Store
}
// An array pattern runs an iterator, and an object pattern can run a getter.
| InstructionValue::Destructure { .. } => Effect::Write,
InstructionValue::Destructure { lvalue, .. } => {
if lvalue.kind == InstructionKind::Reassign {
Effect::Store
} else {
Effect::Write
}
}
// A declaration writes a variable that nothing before it can read.
InstructionValue::StoreLocal { lvalue, .. }
| InstructionValue::StoreContext { lvalue, .. } => {
if lvalue.kind == InstructionKind::Reassign {
Effect::Write
Effect::Store
} else {
Effect::None
}
Expand Down
Loading
Loading