Skip to content

Commit 53afbf0

Browse files
committed
Update on "[compiler][rewrite] PropagateScopeDeps hir rewrite"
\### Quick background: \#### Rvalues / temporaries: In the compiler, unnamed temporaries that represents the evaluation of an expression In the code snippet below, $1, $2, $3, and $4 are temporaries. ```js // input function Component({ bar} ) { const x = {a: foo(bar), b: {}}; } // gets lowered to [1] $2 = LoadGlobal(global) foo [2] $3 = LoadLocal bar$1 [3] $4 = Call $2(<unknown> $3) [4] $5 = Object { } [5] $6 = Object { a: $4, b: $5 } [6] $8 = StoreLocal Const x$7 = $6 ``` The compiler currently treats temporaries and named variables (e.g. `x`) differently in this pass. - named variables may be reassigned (in fact, since we're running after LeaveSSA, a single named identifier's IdentifierId may map to multiple `Identifier` instances -- each with its own scope and mutable range) - temporaries are replaced with their represented expressions during codegen. This is correct (mostly correct, see #29878) as we're careful to always lower the correct evaluation semantics. However, since we rewrite reactive scopes entirely (to if/else blocks), we need to track temporaries that a scope produces in `ReactiveScope.declarations` and later promote them to named variables. In the same example, $4, $5, and $6 need to be promoted: $2 ->`t0`, $5 ->`t1`, and $6 ->`t2`. ```js [1] $2 = LoadGlobal(global) foo [2] $3 = LoadLocal bar$1 scope 0: [3] $4 = Call $2(<unknown> $3) scope 1: [4] $5 = Object { } scope 2: [5] $6 = Object { a: $4, b: $5 } [6] $8 = StoreLocal Const x$7 = $6 ``` \#### Dependencies `ReactiveScope.dependencies` records the set of (read-only) values that a reactive scope is dependent on. This is currently limited to just variables (named variables from source and promoted temporaries) and property-loads. All dependencies we record need to be hoistable -- i.e. reordered to just before the ReactiveScope begins. Not all PropertyLoads are hoistable. In this example, we should not evaluate `obj.a.b` without before creating x and checking `objIsNull`. ```js // reduce-reactive-deps/no-uncond.js function useFoo({ obj, objIsNull }) { const x = []; if (isFalse(objIsNull)) { x.push(obj.a.b); } return x; } ``` While other memoization strategies with different constraints exist, the current compiler requires that `ReactiveScope.dependencies` be re-orderable to the beginning of the reactive scope. But.. `PropertyLoad`s from null values will throw `TypeError`. This means that evaluating hoisted dependencies should throw if and only if the source program throws. (It is also a bug if source throws and compiler output does not throw. See facebook/react-forget#2709) --- \### Rough high level overview 1. Pass 1 Walk over instructions to gather every temporary used outside of its defining scope (same as ReactiveFunction version). These determine the sidemaps we produce, as temporaries used outside of their declaring scopes get promoted to named variables later (and are not considered hoistable rvals). 2. Pass 2 (collectHoistablePropertyLoads) Walk over instructions to generate sidemaps: a. temporary identifier -> named variable and property path (e.g. `$3 -> {obj: props, path: ["a", "b"]}`) b. block -> accessed variables and properties (e.g. `bb0 -> [ {obj: props, path: ["a", "b"]} ]`) c. Walk over control flow graph to understand the set of object and property paths that can be read by each basic block. This analysis: - relies on post-dominator trees - traverses the CFG from entry (producing the set of variables/paths unconditionally evaluated *before* a block). - traverses the CFG from exit (producing the set of variables/paths unconditionally evaluated *after* a block). 4. Pass 3: (collectDependencies) Walks over instructions again to record dependencies and declarations, using the previously produced sidemaps Will add more fixture tests (although most cases should be covered in `reduce-reactive-deps`). Tested by syncing internally and checking compilation output differences ([internal link](https://fburl.com/wiki_markdown/nazsiszd)) --- \### Followups: 1. Rewrite function expression deps This change produces much more optimal output as the compiler now uses the function CFG to understand which variables / paths are assumed to be non-null. However, it may exacerbate [this function-expr hoisting bug](https://github.com/facebook/react/blob/main/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-hoisting-functionexpr.ts). A short term fix here is to simply call some form of `collectNonNullObjects` on every function expression to find hoistable variable / paths. In the longer term, we should refactor out `FunctionExpression.deps`. 2. Enable optional paths (a) don't count optional load temporaries as dependencies (e.g. `collectOptionalLoadRValues(...)`). (b) add optional paths back. This is a bit tricky as we'll want to implement some merging logic for `ConditionalAccess | OptionalChain | UnconditionalAccess`. In addition, our current optional chain lowering is slightly incorrect / imprecise [ghstack-poisoned]
2 parents e0acdd8 + c088ad0 commit 53afbf0

File tree

67 files changed

+1896
-1736
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

67 files changed

+1896
-1736
lines changed

.circleci/config.yml

Lines changed: 2 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ jobs:
281281
at: .
282282
- setup_node_modules
283283
- run: ./scripts/circleci/download_devtools_regression_build.js << parameters.version >> --replaceBuild
284-
- run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci
284+
- run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci=circleci
285285

286286
run_devtools_e2e_tests_for_versions:
287287
docker: *docker
@@ -345,31 +345,6 @@ jobs:
345345
yarn extract-errors
346346
git diff --quiet || (echo "Found unminified errors. Either update the error codes map or disable error minification for the affected build, if appropriate." && false)
347347
348-
check_generated_fizz_runtime:
349-
docker: *docker
350-
environment: *environment
351-
steps:
352-
- checkout
353-
- attach_workspace: *attach_workspace
354-
- setup_node_modules
355-
- run:
356-
name: Confirm generated inline Fizz runtime is up to date
357-
command: |
358-
yarn generate-inline-fizz-runtime
359-
git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false)
360-
361-
yarn_test:
362-
docker: *docker
363-
environment: *environment
364-
parallelism: *TEST_PARALLELISM
365-
parameters:
366-
args:
367-
type: string
368-
steps:
369-
- checkout
370-
- setup_node_modules
371-
- run: yarn test <<parameters.args>> --ci
372-
373348
yarn_test_build:
374349
docker: *docker
375350
environment: *environment
@@ -382,7 +357,7 @@ jobs:
382357
- attach_workspace:
383358
at: .
384359
- setup_node_modules
385-
- run: yarn test --build <<parameters.args>> --ci
360+
- run: yarn test --build <<parameters.args>> --ci=circleci
386361

387362
RELEASE_CHANNEL_stable_yarn_test_dom_fixtures:
388363
docker: *docker
@@ -432,42 +407,6 @@ workflows:
432407
build_and_test:
433408
unless: << pipeline.parameters.prerelease_commit_sha >>
434409
jobs:
435-
- check_generated_fizz_runtime:
436-
filters:
437-
branches:
438-
ignore:
439-
- builds/facebook-www
440-
- yarn_test:
441-
filters:
442-
branches:
443-
ignore:
444-
- builds/facebook-www
445-
matrix:
446-
parameters:
447-
args:
448-
# Intentionally passing these as strings instead of creating a
449-
# separate parameter per CLI argument, since it's easier to
450-
# control/see which combinations we want to run.
451-
- "-r=stable --env=development"
452-
- "-r=stable --env=production"
453-
- "-r=experimental --env=development"
454-
- "-r=experimental --env=production"
455-
- "-r=www-classic --env=development --variant=false"
456-
- "-r=www-classic --env=production --variant=false"
457-
- "-r=www-classic --env=development --variant=true"
458-
- "-r=www-classic --env=production --variant=true"
459-
- "-r=www-modern --env=development --variant=false"
460-
- "-r=www-modern --env=production --variant=false"
461-
- "-r=www-modern --env=development --variant=true"
462-
- "-r=www-modern --env=production --variant=true"
463-
- "-r=xplat --env=development --variant=false"
464-
- "-r=xplat --env=development --variant=true"
465-
- "-r=xplat --env=production --variant=false"
466-
- "-r=xplat --env=production --variant=true"
467-
468-
# TODO: Test more persistent configurations?
469-
- '-r=stable --env=development --persistent'
470-
- '-r=experimental --env=development --persistent'
471410
- yarn_build:
472411
filters:
473412
branches:

.github/workflows/compiler-playground.yml renamed to .github/workflows/compiler_playground.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Compiler Playground
1+
name: (Compiler) Playground
22

33
on:
44
push:

.github/workflows/compiler-rust.yml renamed to .github/workflows/compiler_rust.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: React Compiler (Rust)
1+
name: (Compiler) Rust
22

33
on:
44
push:

.github/workflows/compiler-typescript.yml renamed to .github/workflows/compiler_typescript.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: React Compiler (TypeScript)
1+
name: (Compiler) TypeScript
22

33
on:
44
push:

.github/workflows/devtools_check_repro.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: DevTools Check for bug repro
1+
name: (DevTools) Check for bug repro
22
on:
33
issues:
44
types: [opened, edited]

.github/workflows/commit_artifacts.yml renamed to .github/workflows/runtime_commit_artifacts.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Commit Artifacts for Meta WWW and fbsource
1+
name: (Runtime) Commit Artifacts for Meta WWW and fbsource
22

33
on:
44
push:

.github/workflows/runtime_fizz.yml

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
name: (Runtime) Fizz
2+
3+
on:
4+
push:
5+
branches: [main]
6+
pull_request:
7+
paths-ignore:
8+
- 'compiler/**'
9+
10+
jobs:
11+
check_generated_fizz_runtime:
12+
name: Confirm generated inline Fizz runtime is up to date
13+
runs-on: ubuntu-latest
14+
steps:
15+
- uses: actions/checkout@v4
16+
- uses: actions/setup-node@v4
17+
with:
18+
node-version: 18.x
19+
cache: "yarn"
20+
cache-dependency-path: yarn.lock
21+
- name: Restore cached node_modules
22+
uses: actions/cache@v4
23+
id: node_modules
24+
with:
25+
path: "**/node_modules"
26+
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
27+
- run: yarn install --frozen-lockfile
28+
- run: |
29+
yarn generate-inline-fizz-runtime
30+
git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false)

.github/workflows/flags.yml renamed to .github/workflows/runtime_flags.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Flags
1+
name: (Runtime) Flags
22

33
on:
44
push:

.github/workflows/flow.yml renamed to .github/workflows/runtime_flow.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Flow
1+
name: (Runtime) Flow
22

33
on:
44
push:
@@ -44,4 +44,4 @@ jobs:
4444
path: "**/node_modules"
4545
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
4646
- run: yarn install --frozen-lockfile
47-
- run: node ./scripts/tasks/flow-ci-ghaction ${{ matrix.flow_inline_config_shortname }}
47+
- run: node ./scripts/tasks/flow-ci ${{ matrix.flow_inline_config_shortname }}

.github/workflows/fuzz_tests.yml renamed to .github/workflows/runtime_fuzz_tests.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: facebook/react/fuzz_tests
1+
name: (Runtime) Fuzz tests
22
on:
33
schedule:
44
- cron: 0 * * * *
@@ -28,5 +28,5 @@ jobs:
2828
shell: bash
2929
- name: Run fuzz tests
3030
run: |-
31-
FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci
32-
FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci
31+
FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci=github
32+
FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci=github

.github/workflows/runtime_test.yml

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
name: (Runtime) Test
2+
3+
on:
4+
push:
5+
branches: [main]
6+
pull_request:
7+
paths-ignore:
8+
- 'compiler/**'
9+
10+
env:
11+
# Number of workers (one per shard) to spawn
12+
SHARD_COUNT: 5
13+
14+
jobs:
15+
# Define the various test parameters and parallelism for this workflow
16+
build_test_params:
17+
name: Build test params
18+
runs-on: ubuntu-latest
19+
outputs:
20+
params: ${{ steps.define-params.outputs.result }}
21+
shard_id: ${{ steps.define-shards.outputs.result }}
22+
steps:
23+
- uses: actions/github-script@v7
24+
id: define-shards
25+
with:
26+
script: |
27+
function range(from, to) {
28+
const arr = [];
29+
for (let n = from; n <= to; n++) {
30+
arr.push(n);
31+
}
32+
return arr;
33+
}
34+
return range(1, process.env.SHARD_COUNT);
35+
- uses: actions/github-script@v7
36+
id: define-params
37+
with:
38+
script: |
39+
return [
40+
"-r=stable --env=development",
41+
"-r=stable --env=production",
42+
"-r=experimental --env=development",
43+
"-r=experimental --env=production",
44+
"-r=www-classic --env=development --variant=false",
45+
"-r=www-classic --env=production --variant=false",
46+
"-r=www-classic --env=development --variant=true",
47+
"-r=www-classic --env=production --variant=true",
48+
"-r=www-modern --env=development --variant=false",
49+
"-r=www-modern --env=production --variant=false",
50+
"-r=www-modern --env=development --variant=true",
51+
"-r=www-modern --env=production --variant=true",
52+
"-r=xplat --env=development --variant=false",
53+
"-r=xplat --env=development --variant=true",
54+
"-r=xplat --env=production --variant=false",
55+
"-r=xplat --env=production --variant=true",
56+
// TODO: Test more persistent configurations?
57+
"-r=stable --env=development --persistent",
58+
"-r=experimental --env=development --persistent"
59+
];
60+
61+
# Spawn a job for each shard for a given set of test params
62+
test:
63+
name: yarn test ${{ matrix.params }} (Shard ${{ matrix.shard_id }})
64+
runs-on: ubuntu-latest
65+
needs: build_test_params
66+
strategy:
67+
matrix:
68+
params: ${{ fromJSON(needs.build_test_params.outputs.params) }}
69+
shard_id: ${{ fromJSON(needs.build_test_params.outputs.shard_id) }}
70+
continue-on-error: true
71+
steps:
72+
- uses: actions/checkout@v4
73+
- uses: actions/setup-node@v4
74+
with:
75+
node-version: 18.x
76+
cache: "yarn"
77+
cache-dependency-path: yarn.lock
78+
- name: Restore cached node_modules
79+
uses: actions/cache@v4
80+
id: node_modules
81+
with:
82+
path: "**/node_modules"
83+
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
84+
- run: yarn install --frozen-lockfile
85+
- run: yarn test ${{ matrix.params }} --ci=github --shard=${{ matrix.shard_id }}/${{ env.SHARD_COUNT }}

.github/workflows/lint.yml renamed to .github/workflows/shared_lint.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Lint
1+
name: (Shared) Lint
22

33
on:
44
push:

.github/workflows/stale.yml renamed to .github/workflows/shared_stale.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# Configuration for stale action workflow - https://github.com/actions/stale
2-
name: 'Manage stale issues and PRs'
2+
name: (Shared) Manage stale issues and PRs
33
on:
44
schedule:
55
# Run hourly

compiler/packages/babel-plugin-react-compiler/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "babel-plugin-react-compiler",
3-
"version": "0.0.0-experimental-179941d-20240614",
3+
"version": "0.0.0-experimental-696af53-20240625",
44
"description": "Babel plugin for React Compiler.",
55
"main": "dist/index.js",
66
"license": "MIT",

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ import {
9696
validatePreservedManualMemoization,
9797
validateUseMemo,
9898
} from "../Validation";
99-
import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDepsHIR";
99+
import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDependenciesHIR";
100100

101101
export type CompilerPipelineValue =
102102
| { kind: "ast"; name: string; value: CodegenFunction }

0 commit comments

Comments
 (0)