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.

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

compiler: More precise escape analysis #29782

Closed

Conversation

josephsavona
Copy link
Contributor

@josephsavona josephsavona commented Jun 6, 2024

Stack from ghstack (oldest at bottom):

Our current escape analysis currently works by determing values which are returned, finding the scope those values are produced in, and then walking the dependencies of those scopes. Notably, when we traverse into scopes, we force their values to be memoized — even in cases where we can prove it isn't necessary.

The idea of this PR is to change escape analysis to be purely based on the flow of values. So rather than assume all scope deps need to be force-memoized, we look at how those values are used.

Note: the main motivation for this PR is the follow-up, which allows us to infer dependencies for pruned scopes. Without this change, inferring deps for pruned scopes causes the escape analysis pass to consider values as escaping which shouldn't be.

EDIT: we have to think more about cases like this:

function Component({foo, bar}) {
  const x = makeObject();
  const arr = [];
  if (x) {
    arr.push(foo)
  }
  return arr;
}

where the output on this PR is:

function Component(t0) {
  const $ = _c(2);

  const { foo } = t0;
  const x = makeObject();
  let arr;

  if ($[0] !== foo) {
    arr = [];

    if (x) {
      arr.push(foo);
    }

    $[0] = foo;
    $[1] = arr;
  } else {
    arr = $[1];
  }

  return arr;
}

Note that the const x = makeObject() isn't memoized, because we correctly determine that x doesn't escape. Here, makeObject() should be pure such that it's result never changes, and therefore it should be fine to recalculate x on every render and know that if (x) can't have changed in the memo block. But forcing makeObject() to evaluate once (by memoizing it w/o deps) guarantees that x can't change.

Of course, if you're violating purity and makeObject() can change, than both our current compilation and the changed version from this PR would get errors, ignoring the updating value. But one advantage of our current compilation is that we force all usage of x to get the same, memoized value which feels slightly better due to consistency.

Our current escape analysis currently works by determing values which are returned, finding the scope those values are produced in, and then walking the dependencies of those scopes. Notably, when we traverse into scopes, we _force_ their values to be memoized — even in cases where we can prove it isn't necessary.

The idea of this PR is to change escape analysis to be purely based on the flow of values. So rather than assume all scope deps need to be force-memoized, we look at how those values are used.

Note: the main motivation for this PR is the follow-up, which allows us to infer dependencies for pruned scopes. Without this change, inferring deps for pruned scopes causes the escape analysis pass to consider values as escaping which shouldn't be.

[ghstack-poisoned]
Copy link

vercel bot commented Jun 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2024 4:36pm

@react-sizebot
Copy link

Comparing: eabb681...d9b212d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.26 kB 497.26 kB = 89.11 kB 89.12 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.08 kB 502.08 kB = 89.80 kB 89.80 kB
facebook-www/ReactDOM-prod.classic.js = 594.56 kB 594.56 kB = 104.72 kB 104.72 kB
facebook-www/ReactDOM-prod.modern.js = 570.95 kB 570.95 kB = 101.13 kB 101.13 kB
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 63.89 kB 0.00 kB Deleted 15.97 kB 0.00 kB

Generated by 🚫 dangerJS against d9b212d

Comment on lines +36 to +47
const y = makeObject(a);
let t1;
if ($[2] !== a) {
t1 = makeObject(a);
$[2] = a;
$[3] = t1;
} else {
t1 = $[3];
}
const y = t1;
let t2;
if ($[4] !== x || $[5] !== y.method || $[6] !== b) {
t2 = x[y.method](b);
$[4] = x;
$[5] = y.method;
$[6] = b;
$[7] = t2;
if ($[2] !== x || $[3] !== y.method || $[4] !== b) {
t1 = x[y.method](b);
$[2] = x;
$[3] = y.method;
$[4] = b;
$[5] = t1;
} else {
t2 = $[7];
t1 = $[5];
}
const z = t2;
const z = t1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a perfect example of how this PR makes escape analysis more precise. The dependency is on y.method, which we only use where a primitive is expected (in x[y.method]). We don't have to memoize the makeObject(a) call if we're only going to extract a primitive from it.

Comment on lines +75 to +78
const cond = identity(false);
let t1;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t1 = identity(false);
$[0] = t1;
} else {
t1 = $[0];
}
const cond = t1;
let t2;
if ($[1] !== arr) {
t2 = cond ? { val: CONST_TRUE } : mutate(arr);
$[1] = arr;
$[2] = t2;
if ($[0] !== arr) {
t1 = cond ? { val: CONST_TRUE } : mutate(arr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This output is invalid — note that arr gets mutated in the memo block, but declared outside of it — but that's because this is a bug that is fixed in the new HIR-based reactive scopes mode (which is on by default, this fixture opts out with @enableReactiveScopesInHIR:false). The version of this fixture using HIR-based reactive scopes doesn't change as a result of this PR, and both the arr = shallowCopy(input) and identity(false) are correctly inside the memo block for arr

@josephsavona josephsavona marked this pull request as draft June 6, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants