Add React benchmarking infrastructure #9465

trueadm · 2017-04-20T14:10:10Z

This is currently a WIP and is not yet complete.

The aim of this PR is to give us better tracking of performance and bundle sizes as we make changes to the React codebase.

To do this, new infrastructure is needed to validate our code changes against pre-defined benchmarks. We can do this using Google's Lighthouse tool, performance markers in the benchmarks, an automated build process. For now the benchmarking script does a build and benchmark run comparing the local React repo and a remote merge base React repo.

The end goal is to have this process run on a CI pipeline which can provide us feedback in builds/PRs so we have constant feedback in regards to performance improvements and regressions.

To test how this works, run:

yarn bench

From the branch on this PR. It should take a few minutes to run and report back the performance metrics of local and remote merge base.

There are still things that need to be finished:

Provide a better visual output of results when yarn bench is run
Add bundle size comparison checks and remove the existing code for showing branches from the Rollup build process (it will become redundant with this feature)
Add more benchmarks other than the functional-components benchmark
Add bootstrapped sampled data over results to help show confidence in figures
Add additional CLI options to better refine bench runs

…o add_benchmarking_infra

…infra

trueadm · 2017-05-05T16:43:56Z

@spicyj can you do one more pass on this review before I merge please?

sophiebits · 2017-05-05T16:49:57Z

Sure. Anything in particular you'd like me to focus on?

trueadm · 2017-05-05T16:52:11Z

@spicyj I added a README.md and commented on the commands possible along with some tweaks to the benchmarks. I feel that this is in good shape to merge now – the variance we're seeing is as good as I feel I can get it at this point. If you can see anything I might have missed, please let me know.

gaearon · 2017-05-05T17:08:28Z

(I’m going to look at this now too.)

gaearon · 2017-05-05T17:21:10Z

.eslintignore

@@ -13,6 +13,7 @@ fixtures/
 # Ignore built files.
 build/
 coverage/
-scripts/bench/bench-*.js
+scripts/bench/benchmarks/**/*.js
+scripts/old-bench/bench-*.js


Seems like this can be removed.

gaearon · 2017-05-05T17:21:48Z

.flowconfig

@@ -4,6 +4,8 @@
 <PROJECT_ROOT>/build/.*
 <PROJECT_ROOT>/scripts/.*
 <PROJECT_ROOT>/.*/node_modules/y18n/.*
+<PROJECT_ROOT>/node_modules/chrome-devtools-frontend/.*
+<PROJECT_ROOT>/node_modules/devtools-timeline-model/*


Should this end with .*?

gaearon · 2017-05-05T17:22:36Z

circle.yml

@@ -2,7 +2,7 @@
 machine:
  timezone: America/Los_Angeles
  node:
-    version: 6
+    version: 7


Why did we change this? We're not running benches on CI yet, are we?

Node 7 should be faster than Node 6. Not a big deal though.

gaearon · 2017-05-05T17:23:37Z

scripts/bench/benchmarks/pe-class-components/react-dom.production.min.js

@@ -0,0 +1,7 @@
+/**


Can we avoid checking in built copies of React? People will attempt to edit or download them.

gaearon · 2017-05-05T17:24:53Z

scripts/bench/README.md


-You'll need two folders to compare, each of them containing `react.min.js` and `react-dom-server.min.js`. You can run `npm run build` at the repo root to get a `build` folder with these files.
+```bash
+# will comapre local repo vs remote merge base repo (default run)


Typo: compare (here and below).
What does "(default run)" mean?

gaearon · 2017-05-05T17:25:52Z

package.json

@@ -60,16 +60,22 @@
    "git-branch": "^0.3.0",
    "glob": "^6.0.4",
    "glob-stream": "^6.1.0",
+    "google-closure-compiler-js": "^20170409.0.0",


Is it used currently, or was it added for future experimentation? If not used let's remove it.

Can be removed, was used for static-www build

gaearon · 2017-05-05T17:31:08Z

scripts/bench/runner.js

+async function runRemoteBenchmarks(showResults) {
+  console.log(
+    chalk.white.bold('Running benchmarks for ')
+    + chalk.yellow.bold('Remote (Merge Base)')


Do you think it would be helpful to have a commit hash here?
Or maybe even print a GitHub link to commit below.

gaearon · 2017-05-05T17:48:06Z

Can we avoid the scripts/bench/build/build name? I get confused by the double build and also I'm not sure which build it is. As a comparison, remote-repo is clear to me.

trueadm · 2017-05-05T17:51:08Z

@gaearon I changed the path last week to be scripts/bench/remote-repo already :)

On a side note: all the feedback above has been addressed now!

gaearon · 2017-05-05T18:05:02Z

Ah, I got it now. I just had it from previous version.

gaearon · 2017-05-05T18:05:53Z

scripts/bench/README.md


-For example, if you want to compare a stable verion against master, you can create folders called `build-stable` and `build-master` and use the benchmark scripts like this:
+# will comapre local repo vs remote merge base repo


Nit: type here too

gaearon · 2017-05-05T18:06:14Z

scripts/bench/README.md

-## Running one
-One thing you can do with them is benchmark initial render time for a realistic hierarchy:
+# runs benchmarks with Chrome in headless mode
+yarn bench -- --headless


Is it better? Should it be the default? When would you want to use it?

To me, the results weren't as effective by default. Headless runs everything much faster (it seems) so it was hard to get good feedback and it seems like it's still a new feature. We can revisit adding this by default in the future.

Do they recommend to use it for perf testing?

I've had no official say-so for perf testing. I'll chase someone to get some feedback on using it before enabling it by default.

Meaning the variance was higher?

The variance was all over the place in some runs and better in others. It was too unreliable and no one could tell me as why this is currently the case with headless mode.

gaearon · 2017-05-05T18:06:58Z

scripts/bench/README.md


-You can name folders any way you like, this was just an example.
+# same as "yarn build"


Probably "yarn bench"?

gaearon · 2017-05-05T18:09:33Z

scripts/bench/README.md

-$ ./measure.py build-stable stable.txt build-master master.txt
-$ ./analyze.py stable.txt master.txt
-```
+# will only build and run local repo against benchmarks


I don't fully understand what these two commands do. (--local and --remote)
If I run --local will it not benchmark remote at all? Or will it just skip the build for remote?

If it doesn't run remote, how would I interpret the benchmark? Does it mean I'll only get absolute numbers as there's nothing to compare to?

Yes, it's only for absolute values

gaearon · 2017-05-05T18:13:47Z

scripts/bench/server.js

+const mime = require('mime');
+
+const options = {
+  // key: readFileSync(join(__dirname, 'certs', 'facebook-www.key')),


Can we delete this?

gaearon · 2017-05-05T18:14:25Z

scripts/bench/stats.js

+}
+
+function addBundleSizeComparions(table, localResults, remoteMasterResults) {
+  const bunldesRowHeader = [chalk.white.bold('Bundles')];


Typo: bunldes

gaearon

I ran this a few times, and it behaves mostly as I would expect. Seems like it's important not to switch the active process while the bench is running (e.g. brining Chromium into foreground during one run but not the other significantly skews it).

I did not review the statistical stuff as I trust @spicyj with this. I did not review in detail how you call Lighthouse since you have way more knowledge about this.

I'm approving but please make the instructions for different options clearer. As in "when would I want to run this".

Also please remove the remaining built react files from the repo (there's still a few being added in this PR).

gaearon · 2017-05-05T18:24:15Z

One additional nit is I'd like this to be tweaked:

├───────────────────────────────────────────────────────┼─────────────────────┼────────────────────────┼───────────────────┤
│ Load React                                            │ 394.51 ms +- 1.10   │ 280.76 ms +- 0.62      │ -28.8 % +- 0.3 %  │
├───────────────────────────────────────────────────────┼─────────────────────┼────────────────────────┼───────────────────┤
│ Load ReactDOM                                         │ 1081.16 ms +- 10.86 │ 1152.09 ms +- 1.48     │ +6.6 % +- 1.1 %   │

Note how in this example one of them improved and the other regressed, but it's not clear what the cumulative effect is. It would help to have a Load React + ReactDOM (Total).

Initial commit for WIP benchmarking infrastructure

efdd44a

facebook-github-bot added the CLA Signed label Apr 20, 2017

gaearon changed the title ~~[WIp} Adds React benchmarking infrastructure~~ [WIP] Adds React benchmarking infrastructure Apr 20, 2017

trueadm and others added 11 commits April 20, 2017 15:30

fixed lint issues and ran prettier

f4bc2f7

added <rootDir>/scripts/bench/ to ignore paths for Jest

6c02ebe

Merge master into remote

0b09816

tidied up code and fixed a few bugs in the runner.js

4b7ef00

Merge branch 'add_benchmarking_infra' of github.com:trueadm/react int…

941745a

…o add_benchmarking_infra

fixed eslint

c1e34bc

improved the benchmark output from the runner

fd86c20

fixed typo

d381fb4

tided up print output in runner.js

c1b992f

throw error if chrome canary is not installed on mac

44395a3

fix yarn.lock

632274f

gaearon mentioned this pull request Apr 21, 2017

One-click benchmark runner script #7725

Closed

trueadm added 15 commits April 22, 2017 00:18

added better bench stats output (tables)

efa63c2

added benchmark diff to table results

a1b17c1

adds bundle size comparisons to results

9585a02

Merge remote-tracking branch 'upstream/master' into add_benchmarking_…

1c768b9

…infra

tidied up the results

76584fe

fixed prettier output

5de7727

attempt to trigger bech for circleci build

07901f9

fixed merge conflict

366e4c0

fixes flow exlclusion for lighthouse module

2e354c0

added class components benchmark

083623e

cleaned up stats.js

2a9cb20

stability changes

750f611

circleci node version to 7

414a714

added another benchmark

f53c359

added colours to the different benchmarks to check if being cached

ab2601a

trueadm and others added 3 commits May 3, 2017 15:31

improved the git build process

eaff2e2

added README

8fdafc5

Merge branch 'master' into add_benchmarking_infra

f52440b

gaearon changed the title ~~[WIP] Adds React benchmarking infrastructure~~ Adds React benchmarking infrastructure May 5, 2017

gaearon changed the title ~~Adds React benchmarking infrastructure~~ Add React benchmarking infrastructure May 5, 2017

gaearon reviewed May 5, 2017

View reviewed changes

trueadm added 2 commits May 5, 2017 18:32

updated based feedback from review

d377dfc

adds merge base commit sha

f29856a

gaearon reviewed May 5, 2017

View reviewed changes

gaearon approved these changes May 5, 2017

View reviewed changes

trueadm added 4 commits May 5, 2017 19:37

addressing more PR feedback

fc38448

remove built JS react files

272d6c7

updated .gitignore

6788752

added combined bundle load times to the metrics

f9443ee

trueadm merged commit a7d8ebd into facebook:master May 9, 2017

elas7 mentioned this pull request Nov 6, 2021

Delete unused Circle CI scripts. #22710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add React benchmarking infrastructure #9465

Add React benchmarking infrastructure #9465

trueadm commented Apr 20, 2017 •

edited

Loading

trueadm commented May 5, 2017

sophiebits commented May 5, 2017

trueadm commented May 5, 2017

gaearon commented May 5, 2017

gaearon May 5, 2017

gaearon May 5, 2017

gaearon May 5, 2017

trueadm May 5, 2017

gaearon May 5, 2017

trueadm May 5, 2017

gaearon May 5, 2017

gaearon May 5, 2017 •

edited

Loading

trueadm May 5, 2017

gaearon May 5, 2017

gaearon commented May 5, 2017

trueadm commented May 5, 2017 •

edited

Loading

gaearon commented May 5, 2017

gaearon May 5, 2017

gaearon May 5, 2017

trueadm May 5, 2017

gaearon May 6, 2017

trueadm May 6, 2017

sophiebits May 9, 2017

trueadm May 9, 2017 •

edited

Loading

gaearon May 5, 2017

gaearon May 5, 2017

trueadm May 5, 2017

gaearon May 5, 2017

gaearon May 5, 2017

gaearon left a comment

gaearon commented May 5, 2017 •

edited

Loading


		For example, if you want to compare a stable verion against master, you can create folders called `build-stable` and `build-master` and use the benchmark scripts like this:
		# will comapre local repo vs remote merge base repo


		You can name folders any way you like, this was just an example.
		# same as "yarn build"

Add React benchmarking infrastructure #9465

Add React benchmarking infrastructure #9465

Conversation

trueadm commented Apr 20, 2017 • edited Loading

trueadm commented May 5, 2017

sophiebits commented May 5, 2017

trueadm commented May 5, 2017

gaearon commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented May 5, 2017

trueadm commented May 5, 2017 • edited Loading

gaearon commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented May 5, 2017 • edited Loading

trueadm commented Apr 20, 2017 •

edited

Loading

gaearon May 5, 2017 •

edited

Loading

trueadm commented May 5, 2017 •

edited

Loading

trueadm May 9, 2017 •

edited

Loading

gaearon commented May 5, 2017 •

edited

Loading