🏗Add web performance measurement gulp task #26026
Conversation
|
|
||
| Use `gulp performance-test` to run this test. It measures performance for the current branch compared to the current release. By default, the command first needs to compile minified runtimes from the current branch by executing the `gulp dist` task. To skip this step, use the `--no-compile` flag, but the task will throw an error if the dist files are missing. Only the runtime and components used by documents from the configured URLs are compiled. | ||
|
|
||
| The `config.json` file contains settings. If you set `headless`, you will be able to watch the test run in Chrome. It is not recommended to change `concurrency` (as it does not speed up the test). The default value for `runs` is 100, which takes ~10 minutes to run, and generates results with ~5% margin of error. Differences larger than this should be investigated. |
There was a problem hiding this comment.
Sounds like setting headless means you won't be able to watch the test run, right?
| "runs": 100, | ||
| "concurrency": 1, | ||
| "urls": [ | ||
| "https://amp.cnn.com/cnn/2019/11/19/weather/tropical-storm-sebastien/index.html" |
There was a problem hiding this comment.
There might be some issues using real URLs without getting permission first; better talk to your friendly neighbourhood PM to get this conversation started :)
| path.join(EXPERIMENT_CACHE_PATH, 'v0'), | ||
| ].forEach(dirPath => { | ||
| try { | ||
| fs.mkdirSync(dirPath); |
There was a problem hiding this comment.
try-catch programming is more of a Python thing than JS. You can do fs.mkdirSync(dirPath, {recursive: true}) which doesn't throw an error if the directory exists, or wrap it with if (!fs.existsSync(dirPath))
|
|
||
| try { | ||
| await page.goto(`file:${urlToCachePath(url, version)}`, { | ||
| waitUntil: 'networkidle0', |
There was a problem hiding this comment.
FYI we had some issues with networkidle0, sometimes websites keep an open connection (iframed ads tend to do that) for longer than the default timeout. networkidle2 worked as a compromise for us with visual diffs. If you get lots of errors here, try that.
Might be worth adding logging statements in the catch block to see how often that happens
| window.measureStarted = Date.now(); | ||
| window.largestContentfulPaint = 0; | ||
|
|
||
| const longTaskObserver = new PerformanceObserver(list => |
There was a problem hiding this comment.
dumb question on my part, but is not possible (or maybe its more difficult?) to set this all up with a single observer?
There was a problem hiding this comment.
It should be possible, but then instead of three callbacks that do one thing, there would be one callback that does three things (with a switch or conditional). This seemed cleaner but I'm open to be persuaded otherwise!
kevinkimball
force-pushed
the
perf-test
branch
from
e9de23d
to
27b3011
Compare
kevinkimball
force-pushed
the
perf-test
branch
from
c00a6d0
to
b68d16e
Compare
kevinkimball
force-pushed
the
perf-test
branch
from
e2ee9af
to
e2b82da
Compare
kevinkimball
force-pushed
the
perf-test
branch
from
f5f5766
to
08ea878
Compare
|
|
||
| Use `gulp performance` to run this test. It measures performance for the current branch compared to the current release. By default, the command first needs to compile minified runtimes from the current branch by executing the `gulp dist` task. To skip this step, use the `--nobuild` flag, but the task will throw an error if the dist files are missing. Only the runtime and components used by documents from the configured URLs are compiled. | ||
|
|
||
| The `config.json` file contains settings. If you set `headless` to `false`, you will be able to watch the test run in Chrome. The default value for `runs` is 100, which takes ~10 minutes to run, and generates results with ~5% margin of error. Differences larger than this should be investigated. |
There was a problem hiding this comment.
Can we include the number of runs required to get different levels of confidence in a graph?
| { | ||
| "headless": true, | ||
| "runs": 100, | ||
| "urls": ["https://www.nbcnews.com/news/amp/ncna1103401"] |
There was a problem hiding this comment.
Can we leave this field empty (with instructions to add a URL) or use a default value of amp.dev?
| function downloadToDisk(url, version = CONTROL) { | ||
| touchDirs(); | ||
|
|
||
| return fetch(url) |
There was a problem hiding this comment.
Nit: Moving to async/await would be nice for this code. Including other sync operations using fs.promises.
| const firstPaint = getMetric('first-paint'); | ||
| const firstContentfulPaint = getMetric('first-contentful-paint'); | ||
|
|
||
| function getMaxFirstInputDelay() { |
There was a problem hiding this comment.
Have we opened a ticket with Puppeteer to get this information directly like fcp etc?
There was a problem hiding this comment.
I'm not sure if that's a Puppeteer issue? It's not in Chrome yet, so maybe it's more of a Chrome issue, but I think that is already underway.
| headless, | ||
| args: [ | ||
| '--allow-file-access-from-files', | ||
| '--enable-blink-features=LayoutInstabilityAPI', |
There was a problem hiding this comment.
Nice! Didn't know about this.
|
|
||
| // Excecute the tasks serially | ||
| const [first, ...rest] = tasks; | ||
| await rest.reduce((prev, task) => prev.then(task), first()); |
There was a problem hiding this comment.
Do we need to execute serially?
There was a problem hiding this comment.
I got more consistent results from executing serially, and on my machine saw overall execution time increased with concurrency
| "private": true, | ||
| "name": "performance", | ||
| "version": "0.1.0", | ||
| "description": "Gulp performance", |
There was a problem hiding this comment.
Nit: A bit more of a description would be nice!
There was a problem hiding this comment.
No problem, I just copied the one in the e2e/package.json file here. I'm not really sure how this works with nested packages, is that an idiom or does it have an actual effect? What kind of desc would you expect to see?
There was a problem hiding this comment.
There's no actual effect since this is a private package. I think it's sufficient to make the description something like "Dev-dependencies used by gulp performance".
The original sources are here, feel free to modify them as well if you like:
amphtml/build-system/tasks/visual-diff/package.json
Lines 1 to 11 in fe1fb52
amphtml/build-system/tasks/e2e/package.json
Lines 1 to 14 in fe1fb52
| experiment.toString().padEnd(12), | ||
| percent(control, experiment), | ||
| ].join(' | '), | ||
| `\n${''.padEnd(68, '-')}\n`, |
There was a problem hiding this comment.
What do these magic constants mean?
kevinkimball
force-pushed
the
perf-test
branch
2 times, most recently
from
5f482a3
to
800d533
Compare
| /** | ||
| * @return {!Promise} | ||
| */ | ||
| async function performance() { |
There was a problem hiding this comment.
You could add code here to automatically run yarn in the build-system/tasks/performance directory. E2E tests do it like this:
amphtml/build-system/tasks/e2e/index.js
Lines 38 to 41 in 7f870b9
amphtml/build-system/tasks/e2e/index.js
Lines 73 to 75 in 7f870b9
| @@ -0,0 +1,7 @@ | |||
| # Performance test | |||
|
|
|||
| This test has its own dependencies that must be installed by running `yarn` in this directory before proceeding. | |||
There was a problem hiding this comment.
This can be done automatically. See comment above (in build-system/tasks/performance/index.js).
|
|
||
| This test has its own dependencies that must be installed by running `yarn` in this directory before proceeding. | ||
|
|
||
| Use `gulp performance` to run this test. It measures performance for the current branch compared to the current release. By default, the command first needs to compile minified runtimes from the current branch by executing the `gulp dist` task. To skip this step, use the `--nobuild` flag, but the task will throw an error if the dist files are missing. Only the runtime and components used by documents from the configured URLs are compiled. |
There was a problem hiding this comment.
nit: It's more accurate to say "compile the minified runtime" than "compile minified runtimes".
|
|
||
| const scripts = Array.from(dom.window.document.querySelectorAll('script')); | ||
| for (const script of scripts) { | ||
| if (script.src.startsWith('https://cdn.ampproject.org')) { |
There was a problem hiding this comment.
Instead of hard-coding the cdn url, does it make sense to use urls.cdn or urls.cdnProxyRegex from here so that this code is immune to future changes in the proxy URL?
Lines 37 to 46 in 7f870b9
There was a problem hiding this comment.
Can't find a way to reuse this const because src is using es6 modules and build-system is using node require. However moved to a const.
|
|
||
| const scripts = Array.from(dom.window.document.querySelectorAll('script')); | ||
| for (const script of scripts) { | ||
| if (script.src.startsWith('https://cdn.ampproject.org')) { |
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "private": true, | |||
| "name": "performance", | |||
There was a problem hiding this comment.
nit: For consistency with other tasks, name this gulp-performance.
| "private": true, | ||
| "name": "performance", | ||
| "version": "0.1.0", | ||
| "description": "Gulp performance", |
There was a problem hiding this comment.
There's no actual effect since this is a private package. I think it's sufficient to make the description something like "Dev-dependencies used by gulp performance".
The original sources are here, feel free to modify them as well if you like:
amphtml/build-system/tasks/visual-diff/package.json
Lines 1 to 11 in fe1fb52
amphtml/build-system/tasks/e2e/package.json
Lines 1 to 14 in fe1fb52
| */ | ||
|
|
||
| const fs = require('fs'); | ||
| const puppeteer = require('puppeteer'); |
There was a problem hiding this comment.
I think puppeteer will have to be lazy-required like the visual-diff tests do, in order to prevent the top-level gulp task from looking for it.
amphtml/build-system/tasks/visual-diff/index.js
Lines 761 to 771 in bbd96aa
kevinkimball
force-pushed
the
perf-test
branch
from
0641196
to
b99f041
Compare
| */ | ||
| async function performance() { | ||
| installPackages_(); | ||
| const {headless, runs, urls} = new loadConfig(); |
There was a problem hiding this comment.
Can we add a message here for when no urls are specified or the config file is invalid? Currently with no URLs the error provided is:
[11:04:35] TypeError: getExtensionsFromArg is not a function or its return value is not iterable
at setExtensionsToBuildFromDocuments (/Users/gharbiw/Development/amphtml/build-system/tasks/extension-helpers.js:165:8)
at compileScripts (/Users/gharbiw/Development/amphtml/build-system/tasks/performance/compile-scripts.js:30:5)
at performance (/Users/gharbiw/Development/amphtml/build-system/tasks/performance/index.js:41:9)
at processTicksAndRejections (internal/process/task_queues.js:93:5)
|
A few more observations I had while using this for
|
|
@wassgha Thanks for helping to test. That's a valid observation about large # of URLs, but since it's run serially against multiple URLs I'm not sure if it's worth fixing in this PR vs. just running the task multiple times. With DDOS, not sure what we can do but we could try to change the UA. It would depend on what sort of anti-scraping measures the site is taking. |
kevinkimball
force-pushed
the
perf-test
branch
2 times, most recently
from
9034443
to
6423e15
Compare
| largestContentfulPaint: window.largestContentfulPaint, | ||
| timeToInteractive: getTimeToInteractive(), | ||
| maxFirstInputDelay: getMaxFirstInputDelay(), | ||
| cumulativeLayoutShift: window.cumulativeLayoutShift, |
There was a problem hiding this comment.
I believe this is a percentage with value from 0 to 1 so let's multiply it by 100 otherwise it'll always get rounded up to 0 in the printed table
There was a problem hiding this comment.
"The layout shift (LS) score is equal to the impact fraction multiplied by the distance fraction" per entry, and we sum all the entries together here. I think you are correct that typically this is going to be a low number, although not necessarily between 0-1. Will multiply by 100 for now although not 100% sure that's the easiest solution
.gitignore
Outdated
| @@ -31,3 +31,6 @@ firebase | |||
| firebase.json | |||
| .firebase/ | |||
| src/purifier/dist | |||
| extensions/amp-analytics/0.1/vendors/*.json | |||
There was a problem hiding this comment.
This was removed in 2f76bee
kevinkimball
force-pushed
the
perf-test
branch
from
6423e15
to
61bcb34
Compare
kevinkimball
force-pushed
the
perf-test
branch
from
c30cfba
to
0721b46
Compare
* master: (62 commits) 📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491) Revert 'Move mutator implementations out to a standalone service' (ampproject#26479) Fix syntax error (ampproject#26481) Add pespective back. (ampproject#26486) More user friendly errors in layout.js (ampproject#26448) ✨ Start logging AMP URL on SwG Pages (ampproject#26480) Fix border around desktop amp-story-pages. (ampproject#26449) Fix Story tests. (ampproject#26464) ✨ Performance Measurement Chrome Extension (ampproject#26333) amp-consent restrict iframe fullScreen if no focus (ampproject#26461) Add performance benchmark task (ampproject#26026) ♻️ amp-script: emit warning if zero height and width. (ampproject#26444) ✨ Launch minimal-wrapper native CEv1 (ampproject#26360) ♻️ Lint: include externs (round 2) (ampproject#26446) amp-script: Create 'fill content' container for responsive/fluid (ampproject#26400) amp-consent remove cmp iframe focus (ampproject#26437) Disable macro-after-long-task in inabox. (ampproject#26459) Launch layoutbox-invalidate-on-scroll (ampproject#26430) Add amp-ad support for ByPlay (ampproject#25663) 🏗 Add specific RTV opt-in to experiments.html (ampproject#26434) ...
kevinkimball
changed the title
Add a performance measurement task to help contributors understand the performance impact of their changes. This is a step towards adding a performance check to CI (see #12128).
When run, downloads example AMP documents and loads them using Puppeteer in order to measure metrics that capture the user perceived speed of an AMP site. Compares the working branch against the current production release. Compiles and minifies the working branch, and downloads the production release to avoid measuring network latency. Prints a report to the console. Margin of error is around ~5% when running 100 iterations, which takes about 10-15 minutes (as tested by running the same document against itself).
Example report:
Reference for metrics: