Skip to content

Commit

Permalink
Update on "[compiler][rewrite] PropagateScopeDeps hir rewrite"
Browse files Browse the repository at this point in the history
\### 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]
  • Loading branch information
mofeiZ committed Jul 10, 2024
2 parents e0acdd8 + c088ad0 commit 53afbf0
Show file tree
Hide file tree
Showing 67 changed files with 1,896 additions and 1,736 deletions.
65 changes: 2 additions & 63 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ jobs:
at: .
- setup_node_modules
- run: ./scripts/circleci/download_devtools_regression_build.js << parameters.version >> --replaceBuild
- run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci
- run: node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion << parameters.version >> --ci=circleci

run_devtools_e2e_tests_for_versions:
docker: *docker
Expand Down Expand Up @@ -345,31 +345,6 @@ jobs:
yarn extract-errors
git diff --quiet || (echo "Found unminified errors. Either update the error codes map or disable error minification for the affected build, if appropriate." && false)
check_generated_fizz_runtime:
docker: *docker
environment: *environment
steps:
- checkout
- attach_workspace: *attach_workspace
- setup_node_modules
- run:
name: Confirm generated inline Fizz runtime is up to date
command: |
yarn generate-inline-fizz-runtime
git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false)
yarn_test:
docker: *docker
environment: *environment
parallelism: *TEST_PARALLELISM
parameters:
args:
type: string
steps:
- checkout
- setup_node_modules
- run: yarn test <<parameters.args>> --ci

yarn_test_build:
docker: *docker
environment: *environment
Expand All @@ -382,7 +357,7 @@ jobs:
- attach_workspace:
at: .
- setup_node_modules
- run: yarn test --build <<parameters.args>> --ci
- run: yarn test --build <<parameters.args>> --ci=circleci

RELEASE_CHANNEL_stable_yarn_test_dom_fixtures:
docker: *docker
Expand Down Expand Up @@ -432,42 +407,6 @@ workflows:
build_and_test:
unless: << pipeline.parameters.prerelease_commit_sha >>
jobs:
- check_generated_fizz_runtime:
filters:
branches:
ignore:
- builds/facebook-www
- yarn_test:
filters:
branches:
ignore:
- builds/facebook-www
matrix:
parameters:
args:
# Intentionally passing these as strings instead of creating a
# separate parameter per CLI argument, since it's easier to
# control/see which combinations we want to run.
- "-r=stable --env=development"
- "-r=stable --env=production"
- "-r=experimental --env=development"
- "-r=experimental --env=production"
- "-r=www-classic --env=development --variant=false"
- "-r=www-classic --env=production --variant=false"
- "-r=www-classic --env=development --variant=true"
- "-r=www-classic --env=production --variant=true"
- "-r=www-modern --env=development --variant=false"
- "-r=www-modern --env=production --variant=false"
- "-r=www-modern --env=development --variant=true"
- "-r=www-modern --env=production --variant=true"
- "-r=xplat --env=development --variant=false"
- "-r=xplat --env=development --variant=true"
- "-r=xplat --env=production --variant=false"
- "-r=xplat --env=production --variant=true"

# TODO: Test more persistent configurations?
- '-r=stable --env=development --persistent'
- '-r=experimental --env=development --persistent'
- yarn_build:
filters:
branches:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Compiler Playground
name: (Compiler) Playground

on:
push:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: React Compiler (Rust)
name: (Compiler) Rust

on:
push:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: React Compiler (TypeScript)
name: (Compiler) TypeScript

on:
push:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/devtools_check_repro.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: DevTools Check for bug repro
name: (DevTools) Check for bug repro
on:
issues:
types: [opened, edited]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Commit Artifacts for Meta WWW and fbsource
name: (Runtime) Commit Artifacts for Meta WWW and fbsource

on:
push:
Expand Down
30 changes: 30 additions & 0 deletions .github/workflows/runtime_fizz.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
name: (Runtime) Fizz

on:
push:
branches: [main]
pull_request:
paths-ignore:
- 'compiler/**'

jobs:
check_generated_fizz_runtime:
name: Confirm generated inline Fizz runtime is up to date
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 18.x
cache: "yarn"
cache-dependency-path: yarn.lock
- name: Restore cached node_modules
uses: actions/cache@v4
id: node_modules
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: |
yarn generate-inline-fizz-runtime
git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false)
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Flags
name: (Runtime) Flags

on:
push:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Flow
name: (Runtime) Flow

on:
push:
Expand Down Expand Up @@ -44,4 +44,4 @@ jobs:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: node ./scripts/tasks/flow-ci-ghaction ${{ matrix.flow_inline_config_shortname }}
- run: node ./scripts/tasks/flow-ci ${{ matrix.flow_inline_config_shortname }}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: facebook/react/fuzz_tests
name: (Runtime) Fuzz tests
on:
schedule:
- cron: 0 * * * *
Expand Down Expand Up @@ -28,5 +28,5 @@ jobs:
shell: bash
- name: Run fuzz tests
run: |-
FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci
FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci
FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci=github
FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci=github
85 changes: 85 additions & 0 deletions .github/workflows/runtime_test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
name: (Runtime) Test

on:
push:
branches: [main]
pull_request:
paths-ignore:
- 'compiler/**'

env:
# Number of workers (one per shard) to spawn
SHARD_COUNT: 5

jobs:
# Define the various test parameters and parallelism for this workflow
build_test_params:
name: Build test params
runs-on: ubuntu-latest
outputs:
params: ${{ steps.define-params.outputs.result }}
shard_id: ${{ steps.define-shards.outputs.result }}
steps:
- uses: actions/github-script@v7
id: define-shards
with:
script: |
function range(from, to) {
const arr = [];
for (let n = from; n <= to; n++) {
arr.push(n);
}
return arr;
}
return range(1, process.env.SHARD_COUNT);
- uses: actions/github-script@v7
id: define-params
with:
script: |
return [
"-r=stable --env=development",
"-r=stable --env=production",
"-r=experimental --env=development",
"-r=experimental --env=production",
"-r=www-classic --env=development --variant=false",
"-r=www-classic --env=production --variant=false",
"-r=www-classic --env=development --variant=true",
"-r=www-classic --env=production --variant=true",
"-r=www-modern --env=development --variant=false",
"-r=www-modern --env=production --variant=false",
"-r=www-modern --env=development --variant=true",
"-r=www-modern --env=production --variant=true",
"-r=xplat --env=development --variant=false",
"-r=xplat --env=development --variant=true",
"-r=xplat --env=production --variant=false",
"-r=xplat --env=production --variant=true",
// TODO: Test more persistent configurations?
"-r=stable --env=development --persistent",
"-r=experimental --env=development --persistent"
];
# Spawn a job for each shard for a given set of test params
test:
name: yarn test ${{ matrix.params }} (Shard ${{ matrix.shard_id }})
runs-on: ubuntu-latest
needs: build_test_params
strategy:
matrix:
params: ${{ fromJSON(needs.build_test_params.outputs.params) }}
shard_id: ${{ fromJSON(needs.build_test_params.outputs.shard_id) }}
continue-on-error: true
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 18.x
cache: "yarn"
cache-dependency-path: yarn.lock
- name: Restore cached node_modules
uses: actions/cache@v4
id: node_modules
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: yarn test ${{ matrix.params }} --ci=github --shard=${{ matrix.shard_id }}/${{ env.SHARD_COUNT }}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Lint
name: (Shared) Lint

on:
push:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Configuration for stale action workflow - https://github.com/actions/stale
name: 'Manage stale issues and PRs'
name: (Shared) Manage stale issues and PRs
on:
schedule:
# Run hourly
Expand Down
2 changes: 1 addition & 1 deletion compiler/packages/babel-plugin-react-compiler/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "babel-plugin-react-compiler",
"version": "0.0.0-experimental-179941d-20240614",
"version": "0.0.0-experimental-696af53-20240625",
"description": "Babel plugin for React Compiler.",
"main": "dist/index.js",
"license": "MIT",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ import {
validatePreservedManualMemoization,
validateUseMemo,
} from "../Validation";
import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDepsHIR";
import { propagateScopeDependenciesHIR } from "../HIR/PropagateScopeDependenciesHIR";

export type CompilerPipelineValue =
| { kind: "ast"; name: string; value: CodegenFunction }
Expand Down
Loading

0 comments on commit 53afbf0

Please sign in to comment.