Skip to content

[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees #34346

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
Merged
josephsavona merged 1 commit into from
Sep 4, 2025
Merged

[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees #34346

josephsavona merged 1 commit into from
Sep 4, 2025

Conversation

@josephsavona
Copy link
Member

@josephsavona josephsavona commented Aug 29, 2025 •
edited
Loading

I tried turning on @enablePreserveExistingMemoizationGuarantees by default and cleaned up a couple small things:

  • We emit freeze calls for StartMemoize deps but these had ValueReason.Other so the message wasn't great. We now treat these like other hook arguments.
  • PruneNonEscapingScopes was being too aggressive in this mode and memoizing even loads of globals. Switching to MemoizationLevel.Conditional ensures we build a graph that connects through to primitive-returning function calls, but doesn't unnecessarily force memoization otherwise.

Stack created with Sapling. Best reviewed with ReviewStack.

This was referenced Aug 29, 2025
Merged
Merged
Closed
@meta-cla meta-cla bot added the CLA Signed label Aug 29, 2025
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Aug 29, 2025
@josephsavona josephsavona mentioned this pull request Aug 30, 2025
Closed
josephsavona added a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] Fix for scopes with unreachable fallthroughs (#34335)
5d64f74 
Fixes #34108. If a scope ends with with a conditional where some/all
branches exit via labeled break, we currently compile in a way that
works but bypasses memoization. We end up with a shape like


```js
let t0;
label: {
 if (changed) {
   ...
   if (cond) {
     t0 = ...;
     break label;
   }
   // we don't save the output if the break happens!
   t0 = ...;
   $[0] = t0;
 } else {
   t0 = $[0];
}
```

The fix here is to update AlignReactiveScopesToBlockScopes to take
account of breaks that don't go to the natural fallthrough. In this
case, we take any active scopes and extend them to start at least as
early as the label, and extend at least to the label fallthrough. Thus
we produce the correct:

```js
let t0;
if (changed) {
  label: {
    ...
    if (cond) {
      t0 = ...;
      break label;
    }
    t0 = ...;
  }
  // now the break jumps here, and we cache the value
  $[0] = t0;
} else {
  t0 = $[0];
}
```

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335).
* #34347
* #34346
* #34343
* __->__ #34335
josephsavona added a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] enablePreserveExistingMemo memoizes primitive-returning fu…
735e9ac 
…nctions (#34343)

`@enablePreserveExistingMemoizationGuarantees` mode currently does not
guarantee memoization of primitive-returning functions. We're often able
to infer that a function returns a primitive based on how its result is
used, for example `foo() + 1` or `object[getIndex()]`, and by default we
do not currently memoize computation that produces a primitive. The
reasoning behind this is that the compiler is primarily focused on
stopping cascading updates — it's fine to recompute a primitive since we
can cheaply compare that primitive and avoid unnecessary downstream
recomputation. But we've gotten a lot of feedback that people find this
surprising, and that sometimes the computation can be expensive enough
that it should be memoized.

This PR changes `@enablePreserveExistingMemoizationGuarantees` mode to
ensure that primitive-returning functions get memoized. Other modes will
not memoize these functions. Separately from this we are considering
enabling this mode by default.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34343).
* #34347
* #34346
* __->__ #34343
* #34335
@josephsavona
[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees
0fba073 
I tried turning on `@enablePreserveExistingMemoizationGuarantees` by default and cleaned up a couple small things:

* We emit freeze calls for StartMemoize deps but these had ValueReason.Other so the message wasn't great. We now treat these like other hook arguments.
* PruneNonEscapingScopes was being too aggressive in this mode and memoizing even loads of globals. Switching to MemoizationLevel.Conditional ensures we build a graph that connects through to primitive-returning function calls, but doesn't unnecessarily force memoization otherwise.
github-actions bot pushed a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] Fix for scopes with unreachable fallthroughs (#34335)
7f822e3 
Fixes #34108. If a scope ends with with a conditional where some/all
branches exit via labeled break, we currently compile in a way that
works but bypasses memoization. We end up with a shape like

```js
let t0;
label: {
 if (changed) {
   ...
   if (cond) {
     t0 = ...;
     break label;
   }
   // we don't save the output if the break happens!
   t0 = ...;
   $[0] = t0;
 } else {
   t0 = $[0];
}
```

The fix here is to update AlignReactiveScopesToBlockScopes to take
account of breaks that don't go to the natural fallthrough. In this
case, we take any active scopes and extend them to start at least as
early as the label, and extend at least to the label fallthrough. Thus
we produce the correct:

```js
let t0;
if (changed) {
  label: {
    ...
    if (cond) {
      t0 = ...;
      break label;
    }
    t0 = ...;
  }
  // now the break jumps here, and we cache the value
  $[0] = t0;
} else {
  t0 = $[0];
}
```

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335).
* #34347
* #34346
* #34343
* __->__ #34335

DiffTrain build for [5d64f74](5d64f74)
github-actions bot pushed a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] Fix for scopes with unreachable fallthroughs (#34335)
4a2aee9 
Fixes #34108. If a scope ends with with a conditional where some/all
branches exit via labeled break, we currently compile in a way that
works but bypasses memoization. We end up with a shape like

```js
let t0;
label: {
 if (changed) {
   ...
   if (cond) {
     t0 = ...;
     break label;
   }
   // we don't save the output if the break happens!
   t0 = ...;
   $[0] = t0;
 } else {
   t0 = $[0];
}
```

The fix here is to update AlignReactiveScopesToBlockScopes to take
account of breaks that don't go to the natural fallthrough. In this
case, we take any active scopes and extend them to start at least as
early as the label, and extend at least to the label fallthrough. Thus
we produce the correct:

```js
let t0;
if (changed) {
  label: {
    ...
    if (cond) {
      t0 = ...;
      break label;
    }
    t0 = ...;
  }
  // now the break jumps here, and we cache the value
  $[0] = t0;
} else {
  t0 = $[0];
}
```

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335).
* #34347
* #34346
* #34343
* __->__ #34335

DiffTrain build for [5d64f74](5d64f74)
github-actions bot pushed a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] enablePreserveExistingMemo memoizes primitive-returning fu…
aa31ab1 
…nctions (#34343)

`@enablePreserveExistingMemoizationGuarantees` mode currently does not
guarantee memoization of primitive-returning functions. We're often able
to infer that a function returns a primitive based on how its result is
used, for example `foo() + 1` or `object[getIndex()]`, and by default we
do not currently memoize computation that produces a primitive. The
reasoning behind this is that the compiler is primarily focused on
stopping cascading updates — it's fine to recompute a primitive since we
can cheaply compare that primitive and avoid unnecessary downstream
recomputation. But we've gotten a lot of feedback that people find this
surprising, and that sometimes the computation can be expensive enough
that it should be memoized.

This PR changes `@enablePreserveExistingMemoizationGuarantees` mode to
ensure that primitive-returning functions get memoized. Other modes will
not memoize these functions. Separately from this we are considering
enabling this mode by default.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34343).
* #34347
* #34346
* __->__ #34343
* #34335

DiffTrain build for [735e9ac](735e9ac)
@josephsavona josephsavona merged commit 2710795 into main Sep 4, 2025
21 of 30 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees (#…
666c908 
…34346)

I tried turning on `@enablePreserveExistingMemoizationGuarantees` by
default and cleaned up a couple small things:

* We emit freeze calls for StartMemoize deps but these had
ValueReason.Other so the message wasn't great. We now treat these like
other hook arguments.
* PruneNonEscapingScopes was being too aggressive in this mode and
memoizing even loads of globals. Switching to
MemoizationLevel.Conditional ensures we build a graph that connects
through to primitive-returning function calls, but doesn't unnecessarily
force memoization otherwise.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34346).
* #34347
* __->__ #34346

DiffTrain build for [2710795](2710795)
github-actions bot pushed a commit that referenced this pull request Sep 4, 2025
@josephsavona
[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees (#…
7ac0c33 
…34346)

I tried turning on `@enablePreserveExistingMemoizationGuarantees` by
default and cleaned up a couple small things:

* We emit freeze calls for StartMemoize deps but these had
ValueReason.Other so the message wasn't great. We now treat these like
other hook arguments.
* PruneNonEscapingScopes was being too aggressive in this mode and
memoizing even loads of globals. Switching to
MemoizationLevel.Conditional ensures we build a graph that connects
through to primitive-returning function calls, but doesn't unnecessarily
force memoization otherwise.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34346).
* #34347
* __->__ #34346

DiffTrain build for [2710795](2710795)
EugeneChoi4 pushed a commit to EugeneChoi4/react that referenced this pull request Sep 4, 2025
@josephsavona @EugeneChoi4
[compiler] Fix for scopes with unreachable fallthroughs (facebook#34335)
cb7a216 
Fixes facebook#34108. If a scope ends with with a conditional where some/all
branches exit via labeled break, we currently compile in a way that
works but bypasses memoization. We end up with a shape like


```js
let t0;
label: {
 if (changed) {
   ...
   if (cond) {
     t0 = ...;
     break label;
   }
   // we don't save the output if the break happens!
   t0 = ...;
   $[0] = t0;
 } else {
   t0 = $[0];
}
```

The fix here is to update AlignReactiveScopesToBlockScopes to take
account of breaks that don't go to the natural fallthrough. In this
case, we take any active scopes and extend them to start at least as
early as the label, and extend at least to the label fallthrough. Thus
we produce the correct:

```js
let t0;
if (changed) {
  label: {
    ...
    if (cond) {
      t0 = ...;
      break label;
    }
    t0 = ...;
  }
  // now the break jumps here, and we cache the value
  $[0] = t0;
} else {
  t0 = $[0];
}
```

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34335).
* facebook#34347
* facebook#34346
* facebook#34343
* __->__ facebook#34335
EugeneChoi4 pushed a commit to EugeneChoi4/react that referenced this pull request Sep 4, 2025
@josephsavona @EugeneChoi4
[compiler] enablePreserveExistingMemo memoizes primitive-returning fu…
48bb7d3 
…nctions (facebook#34343)

`@enablePreserveExistingMemoizationGuarantees` mode currently does not
guarantee memoization of primitive-returning functions. We're often able
to infer that a function returns a primitive based on how its result is
used, for example `foo() + 1` or `object[getIndex()]`, and by default we
do not currently memoize computation that produces a primitive. The
reasoning behind this is that the compiler is primarily focused on
stopping cascading updates — it's fine to recompute a primitive since we
can cheaply compare that primitive and avoid unnecessary downstream
recomputation. But we've gotten a lot of feedback that people find this
surprising, and that sometimes the computation can be expensive enough
that it should be memoized.

This PR changes `@enablePreserveExistingMemoizationGuarantees` mode to
ensure that primitive-returning functions get memoized. Other modes will
not memoize these functions. Separately from this we are considering
enabling this mode by default.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34343).
* facebook#34347
* facebook#34346
* __->__ facebook#34343
* facebook#34335
EugeneChoi4 pushed a commit to EugeneChoi4/react that referenced this pull request Sep 4, 2025
@josephsavona @EugeneChoi4
[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees (f…
c0836e7 
…acebook#34346)

I tried turning on `@enablePreserveExistingMemoizationGuarantees` by
default and cleaned up a couple small things:

* We emit freeze calls for StartMemoize deps but these had
ValueReason.Other so the message wasn't great. We now treat these like
other hook arguments.
* PruneNonEscapingScopes was being too aggressive in this mode and
memoizing even loads of globals. Switching to
MemoizationLevel.Conditional ensures we build a graph that connects
through to primitive-returning function calls, but doesn't unnecessarily
force memoization otherwise.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34346).
* facebook#34347
* __->__ facebook#34346
@nextjs-bot nextjs-bot mentioned this pull request Sep 4, 2025
Merged
This was referenced Sep 4, 2025
Merged
Merged
josephsavona added a commit that referenced this pull request Sep 6, 2025
@josephsavona
[compiler] Add missing source locations to statements, expressions (#…
f5e96b9 
…34406)

Adds missing locations to all the statement kinds that we produce in
codegenInstruction(), and adds generic handling of source locations for
the nodes produced by codegenInstructionValue(). There are definitely
some places where we are still missing a location, but this should
address some of the known issues we've seen such as missing location on
`throw`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34406).
* #34394
* __->__ #34406
* #34346
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
@josephsavona
[compiler] Add missing source locations to statements, expressions (#…
8aa9685 
…34406)

Adds missing locations to all the statement kinds that we produce in
codegenInstruction(), and adds generic handling of source locations for
the nodes produced by codegenInstructionValue(). There are definitely
some places where we are still missing a location, but this should
address some of the known issues we've seen such as missing location on
`throw`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34406).
* #34394
* __->__ #34406
* #34346

DiffTrain build for [f5e96b9](f5e96b9)
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
@josephsavona
[compiler] Add missing source locations to statements, expressions (#…
a47f447 
…34406)

Adds missing locations to all the statement kinds that we produce in
codegenInstruction(), and adds generic handling of source locations for
the nodes produced by codegenInstructionValue(). There are definitely
some places where we are still missing a location, but this should
address some of the known issues we've seen such as missing location on
`throw`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34406).
* #34394
* __->__ #34406
* #34346

DiffTrain build for [f5e96b9](f5e96b9)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Sep 7, 2025
@pull
[compiler] Cleanup for @enablePreserveExistingMemoizationGuarantees (f…
8d5f9bc 
…acebook#34346)

I tried turning on `@enablePreserveExistingMemoizationGuarantees` by
default and cleaned up a couple small things:

* We emit freeze calls for StartMemoize deps but these had
ValueReason.Other so the message wasn't great. We now treat these like
other hook arguments.
* PruneNonEscapingScopes was being too aggressive in this mode and
memoizing even loads of globals. Switching to
MemoizationLevel.Conditional ensures we build a graph that connects
through to primitive-returning function calls, but doesn't unnecessarily
force memoization otherwise.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34346).
* facebook#34347
* __->__ facebook#34346

DiffTrain build for [2710795](facebook@2710795)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Sep 7, 2025
@pull
[compiler] Add missing source locations to statements, expressions (f…
c2bb6c0 
…acebook#34406)

Adds missing locations to all the statement kinds that we produce in
codegenInstruction(), and adds generic handling of source locations for
the nodes produced by codegenInstructionValue(). There are definitely
some places where we are still missing a location, but this should
address some of the known issues we've seen such as missing location on
`throw`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34406).
* facebook#34394
* __->__ facebook#34406
* facebook#34346

DiffTrain build for [f5e96b9](facebook@f5e96b9)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Sep 7, 2025
@pull
[compiler] Add missing source locations to statements, expressions (f…
92e4367 
…acebook#34406)

Adds missing locations to all the statement kinds that we produce in
codegenInstruction(), and adds generic handling of source locations for
the nodes produced by codegenInstructionValue(). There are definitely
some places where we are still missing a location, but this should
address some of the known issues we've seen such as missing location on
`throw`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34406).
* facebook#34394
* __->__ facebook#34406
* facebook#34346

DiffTrain build for [f5e96b9](facebook@f5e96b9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@josephsavona