Skip to content

Commit

Permalink
Update base for Update on "[compiler] Optimize instruction reordering"
Browse files Browse the repository at this point in the history
Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain.

This PR includes two changes:
* First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once
* Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks.

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.

This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.

So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.

I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures.

[ghstack-poisoned]
  • Loading branch information
josephsavona committed Jun 21, 2024
2 parents 9316d4a + 6aea169 commit dac0b62
Show file tree
Hide file tree
Showing 92 changed files with 415 additions and 270 deletions.
47 changes: 0 additions & 47 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,38 +74,6 @@ parameters:
default: ''

jobs:
yarn_lint:
docker: *docker
environment: *environment

steps:
- checkout
- setup_node_modules
- run: node ./scripts/prettier/index
- run: node ./scripts/tasks/eslint
- run: ./scripts/circleci/check_license.sh
- run: ./scripts/circleci/test_print_warnings.sh

yarn_flow:
docker: *docker
environment: *environment
parallelism: 5

steps:
- checkout
- setup_node_modules
- run: node ./scripts/tasks/flow-ci


yarn_flags:
docker: *docker
environment: *environment

steps:
- checkout
- setup_node_modules
- run: yarn flags

scrape_warning_messages:
docker: *docker
environment: *environment
Expand Down Expand Up @@ -464,26 +432,11 @@ workflows:
build_and_test:
unless: << pipeline.parameters.prerelease_commit_sha >>
jobs:
- yarn_flags:
filters:
branches:
ignore:
- builds/facebook-www
- yarn_flow:
filters:
branches:
ignore:
- builds/facebook-www
- check_generated_fizz_runtime:
filters:
branches:
ignore:
- builds/facebook-www
- yarn_lint:
filters:
branches:
ignore:
- builds/facebook-www
- yarn_test:
filters:
branches:
Expand Down
2 changes: 2 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ module.exports = {
// Stop ESLint from looking for a configuration file in parent folders
root: true,

reportUnusedDisableDirectives: true,

plugins: [
'babel',
'ft-flow',
Expand Down
19 changes: 0 additions & 19 deletions .github/workflows/compiler-typescript.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,6 @@ jobs:
- id: set-matrix
run: echo "matrix=$(find packages -mindepth 1 -maxdepth 1 -type d | sed 's!packages/!!g' | tr '\n' ',' | sed s/.$// | jq -Rsc '. / "," - [""]')" >> $GITHUB_OUTPUT

# Hardcoded to improve parallelism for babel-plugin-react-compiler
prettier:
name: Run prettier
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 18.x
cache: "yarn"
cache-dependency-path: compiler/yarn.lock
- name: Restore cached node_modules
uses: actions/cache@v4
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('compiler/**/yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: yarn prettier:ci

# Hardcoded to improve parallelism
lint:
name: Lint babel-plugin-react-compiler
Expand Down
28 changes: 28 additions & 0 deletions .github/workflows/flags.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: Flags

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

jobs:
flags:
name: Check flags
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 flags
47 changes: 47 additions & 0 deletions .github/workflows/flow.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: Flow

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

jobs:
discover_flow_inline_configs:
name: Discover flow inline configs
runs-on: ubuntu-latest
outputs:
matrix: ${{ steps.set-matrix.outputs.result }}
steps:
- uses: actions/checkout@v4
- uses: actions/github-script@v7
id: set-matrix
with:
script: |
const inlinedHostConfigs = require('./scripts/shared/inlinedHostConfigs.js');
return inlinedHostConfigs.map(config => config.shortName);
flow:
name: Flow check ${{ matrix.flow_inline_config_shortname }}
needs: discover_flow_inline_configs
runs-on: ubuntu-latest
continue-on-error: true
strategy:
matrix:
flow_inline_config_shortname: ${{ fromJSON(needs.discover_flow_inline_configs.outputs.matrix) }}
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: node ./scripts/tasks/flow-ci-ghaction ${{ matrix.flow_inline_config_shortname }}
79 changes: 79 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
name: Lint

on:
push:
branches: [main]
pull_request:

jobs:
prettier:
name: Run prettier
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
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('**/yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: yarn prettier-check

eslint:
name: Run eslint
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
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: node ./scripts/tasks/eslint

check_license:
name: Check license
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
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: ./scripts/circleci/check_license.sh

test_print_warnings:
name: Test print warnings
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
with:
path: "**/node_modules"
key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }}
- run: yarn install --frozen-lockfile
- run: ./scripts/circleci/test_print_warnings.sh
24 changes: 24 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# react runtime
build

packages/react-devtools-core/dist
Expand All @@ -13,3 +14,26 @@ packages/react-devtools-shared/src/hooks/__tests__/__source__/__untransformed__/
packages/react-devtools-shell/dist
packages/react-devtools-timeline/dist
packages/react-devtools-timeline/static

# react compiler
compiler/**/dist
compiler/**/__tests__/fixtures/**/*.expect.md
compiler/**/__tests__/fixtures/**/*.flow.js
compiler/**/.next

compiler/crates
compiler/apps/playground/public

compiler/**/LICENSE
compiler/.*
compiler/*.md*
compiler/*.json
compiler/*.css
compiler/*.webmanifest
compiler/*.map
compiler/*.sh
compiler/*.txt
compiler/*.ico
compiler/*.svg
compiler/*.lock
compiler/*.toml
15 changes: 15 additions & 0 deletions .prettierrc.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const {
compilerPaths,
esNextPaths,
typescriptPaths,
} = require('./scripts/shared/pathsByLanguageVersion');
Expand Down Expand Up @@ -33,5 +34,19 @@ module.exports = {
parser: 'typescript',
},
},
{
files: compilerPaths,
options: {
requirePragma: false,
parser: 'babel-ts',
semi: true,
singleQuote: false,
trailingComma: 'es5',
bracketSpacing: true,
bracketSameLine: false,
printWidth: 80,
arrowParens: 'always',
},
},
],
};
8 changes: 7 additions & 1 deletion compiler/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ module.exports = {

"multiline-comment-style": ["error", "starred-block"],

/**
* We sometimes need to check for control characters in regexes for things like preserving input
* strings
*/
"no-control-regex": "off",

"@typescript-eslint/no-empty-function": "off",

/*
Expand Down Expand Up @@ -82,7 +88,7 @@ module.exports = {
],
"@typescript-eslint/array-type": ["error", { default: "generic" }],
"@typescript-eslint/triple-slash-reference": "off",
"@typescript-eslint/no-var-requires": "off"
"@typescript-eslint/no-var-requires": "off",
},
parser: "@typescript-eslint/parser",
plugins: ["@typescript-eslint"],
Expand Down
21 changes: 0 additions & 21 deletions compiler/.prettierignore

This file was deleted.

9 changes: 0 additions & 9 deletions compiler/.prettierrc.js

This file was deleted.

8 changes: 6 additions & 2 deletions compiler/apps/playground/components/TabbedWindow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ function TabbedWindowItem({
title="Minimize tab"
aria-label="Minimize tab"
onClick={toggleTabs}
className={`p-4 duration-150 ease-in border-b cursor-pointer border-grey-200 ${hasChanged ? "font-bold" : "font-light"} text-secondary hover:text-link`}
className={`p-4 duration-150 ease-in border-b cursor-pointer border-grey-200 ${
hasChanged ? "font-bold" : "font-light"
} text-secondary hover:text-link`}
>
- {name}
</h2>
Expand All @@ -91,7 +93,9 @@ function TabbedWindowItem({
aria-label={`Expand compiler tab: ${name}`}
style={{ transform: "rotate(90deg) translate(-50%)" }}
onClick={toggleTabs}
className={`flex-grow-0 w-5 transition-colors duration-150 ease-in ${hasChanged ? "font-bold" : "font-light"} text-secondary hover:text-link`}
className={`flex-grow-0 w-5 transition-colors duration-150 ease-in ${
hasChanged ? "font-bold" : "font-light"
} text-secondary hover:text-link`}
>
{name}
</button>
Expand Down
Loading

0 comments on commit dac0b62

Please sign in to comment.