Skip to content

Commit

Permalink
compiler: Known hooks/nonescaping scopes dont count as pruned
Browse files Browse the repository at this point in the history
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope:
* Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them.
* Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case.

I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement.

ghstack-source-id: b7c36b71aa44a7e4952670b5121c15fe9c96b123
Pull Request resolved: #29820
  • Loading branch information
josephsavona committed Jun 8, 2024
1 parent 4fa8fa7 commit 5079b56
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,20 @@ class Transform extends ReactiveFunctionTransform<State> {
this.visitScope(scope, innerState);
outerState.hasHook ||= innerState.hasHook;
if (innerState.hasHook) {
if (scope.instructions.length === 1) {
/*
* This was a scope just for a hook call, which doesn't need memoization.
* flatten it away
*/
return {
kind: "replace-many",
value: scope.instructions,
};
}
/*
* else this scope had multiple instructions and produced some other value:
* mark it as pruned
*/
return {
kind: "replace",
value: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -951,12 +951,8 @@ class PruneScopesTransform extends ReactiveFunctionTransform<
} else {
this.prunedScopes.add(scopeBlock.scope.id);
return {
kind: "replace",
value: {
kind: "pruned-scope",
scope: scopeBlock.scope,
instructions: scopeBlock.instructions,
},
kind: "replace-many",
value: scopeBlock.instructions,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@

## Input

```javascript
// @flow
function Component() {
return (
<div
className={stylex(
// this value is a) in its own scope, b) non-reactive, and c) non-escaping
// its scope gets pruned bc it's non-escaping, but this doesn't mean we need to
// create a temporary for it
flags.feature("feature-name") ? styles.featureNameStyle : null
)}
></div>
);
}

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime";
function Component() {
const $ = _c(1);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = (
<div
className={stylex(
flags.feature("feature-name") ? styles.featureNameStyle : null,
)}
/>
);
$[0] = t0;
} else {
t0 = $[0];
}
return t0;
}

```
### Eval output
(kind: exception) Fixture not implemented
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// @flow
function Component() {
return (
<div
className={stylex(
// this value is a) in its own scope, b) non-reactive, and c) non-escaping
// its scope gets pruned bc it's non-escaping, but this doesn't mean we need to
// create a temporary for it
flags.feature("feature-name") ? styles.featureNameStyle : null
)}
></div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const FIXTURE_ENTRYPOINT = {
```javascript
import { c as _c } from "react/compiler-runtime";
function Component(props) {
const $ = _c(2);
const $ = _c(1);

const a = [];
const b = [];
Expand All @@ -53,12 +53,11 @@ function Component(props) {
x = 1;
}
let t0;
if ($[0] !== x) {
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = [x];
$[0] = x;
$[1] = t0;
$[0] = t0;
} else {
t0 = $[1];
t0 = $[0];
}
return t0;
}
Expand Down

0 comments on commit 5079b56

Please sign in to comment.