From c59c3355ea5c159d772861174776668584b4c1ea Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sat, 8 Jun 2024 16:10:43 -0700 Subject: [PATCH 1/3] compiler: Known hooks/nonescaping scopes dont count as pruned 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-poisoned] --- .../FlattenScopesWithHooksOrUse.ts | 10 ++++ .../ReactiveScopes/PruneNonEscapingScopes.ts | 8 +--- ...-can-inline-into-consuming-scope.expect.md | 46 +++++++++++++++++++ ...endency-can-inline-into-consuming-scope.js | 13 ++++++ ...om-interleaved-reactivity-for-of.expect.md | 9 ++-- 5 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts index 06282baa0e362..d7f2ad42daad3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts @@ -66,6 +66,16 @@ class Transform extends ReactiveFunctionTransform { 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: { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index 205eb07c7dd7d..da7d71c6dd99b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -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, }; } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.expect.md new file mode 100644 index 0000000000000..1e380f2fde693 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.expect.md @@ -0,0 +1,46 @@ + +## Input + +```javascript +// @flow +function Component() { + return ( +
+ ); +} + +``` + +## 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 = ( +
+ ); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.js new file mode 100644 index 0000000000000..4b2f3e47cd6f8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonreactive-noescaping-dependency-can-inline-into-consuming-scope.js @@ -0,0 +1,13 @@ +// @flow +function Component() { + return ( +
+ ); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-for-of.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-for-of.expect.md index a8b2da0491eae..e0441ff680e5c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-for-of.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-from-interleaved-reactivity-for-of.expect.md @@ -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 = []; @@ -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; } From 21724a5982416fddffc3f94e523c3e1d9ca24da5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Sat, 8 Jun 2024 16:16:25 -0700 Subject: [PATCH 2/3] Update on "compiler: Known hooks/nonescaping scopes dont count as pruned" 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-poisoned] --- .../ReactiveScopes/FlattenScopesWithHooksOrUse.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts index d7f2ad42daad3..7249801996d11 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts @@ -67,15 +67,19 @@ class Transform extends ReactiveFunctionTransform { 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 + /* + * 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 + /* + * else this scope had multiple instructions and produced some other value: + * mark it as pruned + */ return { kind: "replace", value: { From 8241eec6b85e4facf3857220784abdcea64ad21c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 10 Jun 2024 08:35:30 -0700 Subject: [PATCH 3/3] Update on "compiler: Known hooks/nonescaping scopes dont count as pruned" 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-poisoned] --- .../ReactiveScopes/CodegenReactiveFunction.ts | 25 +----- .../compiler/log-pruned-memoization.expect.md | 77 +++++++++++++++---- .../compiler/log-pruned-memoization.js | 25 +++++- 3 files changed, 83 insertions(+), 44 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 88fff5cfe5156..a6bd55ea9db60 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -331,29 +331,8 @@ class CountMemoBlockVisitor extends ReactiveFunctionVisitor { scopeBlock: PrunedReactiveScopeBlock, state: void ): void { - let isHookOnlyMemoBlock = false; - if ( - scopeBlock.instructions.length === 1 && - scopeBlock.instructions[0].kind === "instruction" - ) { - const instr = scopeBlock.instructions[0]!.instruction; - if ( - instr.value.kind === "MethodCall" || - instr.value.kind === "CallExpression" - ) { - const callee = - instr.value.kind === "MethodCall" - ? instr.value.property - : instr.value.callee; - if (getHookKind(this.env, callee.identifier) != null) { - isHookOnlyMemoBlock = true; - } - } - } - if (!isHookOnlyMemoBlock) { - this.prunedMemoBlocks += 1; - this.prunedMemoValues += scopeBlock.scope.declarations.size; - } + this.prunedMemoBlocks += 1; + this.prunedMemoValues += scopeBlock.scope.declarations.size; this.traversePrunedScope(scopeBlock, state); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.expect.md index d1aeeb608478e..b2b600892f11c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.expect.md @@ -3,10 +3,17 @@ ```javascript // @logger -import { useState } from "react"; -import { identity, makeObject_Primitives, useHook } from "shared-runtime"; +import { createContext, use, useState } from "react"; +import { + Stringify, + identity, + makeObject_Primitives, + useHook, +} from "shared-runtime"; function Component() { + const w = use(Context); + // The scopes for x and x2 are interleaved, so this is one scope with two values const x = makeObject_Primitives(); const x2 = makeObject_Primitives(); @@ -26,11 +33,21 @@ function Component() { } // Overall we expect two pruned scopes (for x+x2, and obj), with 3 pruned scope values. - return [x, x2, y, z]; + return ; +} + +const Context = createContext(); + +function Wrapper() { + return ( + + + + ); } export const FIXTURE_ENTRYPOINT = { - fn: Component, + fn: Wrapper, params: [{}], }; @@ -40,11 +57,17 @@ export const FIXTURE_ENTRYPOINT = { ```javascript import { c as _c } from "react/compiler-runtime"; // @logger -import { useState } from "react"; -import { identity, makeObject_Primitives, useHook } from "shared-runtime"; +import { createContext, use, useState } from "react"; +import { + Stringify, + identity, + makeObject_Primitives, + useHook, +} from "shared-runtime"; function Component() { - const $ = _c(5); + const $ = _c(6); + const w = use(Context); const x = makeObject_Primitives(); const x2 = makeObject_Primitives(); @@ -65,20 +88,39 @@ function Component() { z = $[0]; } let t0; - if ($[1] !== x || $[2] !== x2 || $[3] !== y) { - t0 = [x, x2, y, z]; - $[1] = x; - $[2] = x2; - $[3] = y; - $[4] = t0; + if ($[1] !== w || $[2] !== x || $[3] !== x2 || $[4] !== y) { + t0 = ; + $[1] = w; + $[2] = x; + $[3] = x2; + $[4] = y; + $[5] = t0; + } else { + t0 = $[5]; + } + return t0; +} + +const Context = createContext(); + +function Wrapper() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = ( + + + + ); + $[0] = t0; } else { - t0 = $[4]; + t0 = $[0]; } return t0; } export const FIXTURE_ENTRYPOINT = { - fn: Component, + fn: Wrapper, params: [{}], }; @@ -87,8 +129,9 @@ export const FIXTURE_ENTRYPOINT = { ## Logs ``` -{"kind":"CompileSuccess","fnLoc":{"start":{"line":5,"column":0,"index":121},"end":{"line":26,"column":1,"index":813},"filename":"log-pruned-memoization.ts"},"fnName":"Component","memoSlots":5,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":2,"prunedMemoValues":3} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":10,"column":0,"index":161},"end":{"line":33,"column":1,"index":905},"filename":"log-pruned-memoization.ts"},"fnName":"Component","memoSlots":6,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":2,"prunedMemoValues":3} +{"kind":"CompileSuccess","fnLoc":{"start":{"line":37,"column":0,"index":941},"end":{"line":43,"column":1,"index":1039},"filename":"log-pruned-memoization.ts"},"fnName":"Wrapper","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0} ``` ### Eval output -(kind: ok) [{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},[{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true}]] \ No newline at end of file +(kind: ok)
{"items":[42,{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},[{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true},{"a":0,"b":"value1","c":true}]]}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.js index 2cfb960ce32bd..6b93702211342 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/log-pruned-memoization.js @@ -1,8 +1,15 @@ // @logger -import { useState } from "react"; -import { identity, makeObject_Primitives, useHook } from "shared-runtime"; +import { createContext, use, useState } from "react"; +import { + Stringify, + identity, + makeObject_Primitives, + useHook, +} from "shared-runtime"; function Component() { + const w = use(Context); + // The scopes for x and x2 are interleaved, so this is one scope with two values const x = makeObject_Primitives(); const x2 = makeObject_Primitives(); @@ -22,10 +29,20 @@ function Component() { } // Overall we expect two pruned scopes (for x+x2, and obj), with 3 pruned scope values. - return [x, x2, y, z]; + return ; +} + +const Context = createContext(); + +function Wrapper() { + return ( + + + + ); } export const FIXTURE_ENTRYPOINT = { - fn: Component, + fn: Wrapper, params: [{}], };