Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental image post-processing #15875

Merged
merged 62 commits into from
Aug 5, 2020

Conversation

atcastle
Copy link
Collaborator

@atcastle atcastle commented Aug 4, 2020

This PR adds a second experimental post-processing step for the framework introduced by @prateekbh in #14746. The image post-processing step scans the rendered document for the first few images and uses a simple heuristic to determine if the images should be automatically preloaded.

Analysis of quite a few production Next apps has shown that a lot of sites are taking a substantial hit to their LCP score because an image that's part of the "hero" element on the page is not preloaded and is getting downloaded with lower priority than the JavaScript bundles. This post-processor should automatically fix that for a lot of sites, without causing any real performance effects in cases where it fails to identify the hero image.

This feature is behind an experimental flag, and will be subject to quite a bit of experimentation and tweaking before it's ready to be made a default setting.

@ijjk
Copy link
Member

ijjk commented Aug 4, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
buildDuration 13.4s 13.6s ⚠️ +212ms
nodeModulesSize 65.5 MB 65.5 MB ⚠️ +9.17 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
/ failed reqs 0 0
/ total time (seconds) 2.578 2.66 ⚠️ +0.08
/ avg req/sec 969.59 939.7 ⚠️ -29.89
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.534 1.64 ⚠️ +0.11
/error-in-render avg req/sec 1630.18 1524.51 ⚠️ -105.67
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..afbf.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
main-2ecb3b8..bd63.js gzip 6.73 kB 6.73 kB
webpack-ccf5..276a.js gzip 751 B 751 B
Overall change 56.8 kB 56.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
main-dbff266..dule.js gzip 5.81 kB 5.81 kB
webpack-10c7..dule.js gzip 751 B 751 B
Overall change 51.8 kB 51.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
polyfills-75..1629.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB
hooks-585f07..95a3.js gzip 887 B 887 B
index-c7b63f..fc02.js gzip 227 B 227 B
link-e97733c..af21.js gzip 1.3 kB 1.3 kB
routerDirect..ebc7.js gzip 284 B 284 B
withRouter-2..db68.js gzip 284 B 284 B
Overall change 7.72 kB 7.72 kB
Client Pages Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-97e743e..dule.js gzip 626 B 626 B
_error-b4004..dule.js gzip 2.3 kB 2.3 kB
hooks-696209..dule.js gzip 387 B 387 B
index-a4dd74..dule.js gzip 226 B 226 B
link-fbb1d2e..dule.js gzip 1.25 kB 1.25 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-1..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_buildManifest.js gzip 272 B 272 B
_buildManife..dule.js gzip 280 B 280 B
Overall change 552 B 552 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js add-image-postprocess Change
index.html gzip 944 B 944 B
link.html gzip 953 B 953 B
withRouter.html gzip 940 B 940 B
Overall change 2.84 kB 2.84 kB

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
buildDuration 15.2s 15.3s ⚠️ +124ms
nodeModulesSize 65.5 MB 65.5 MB ⚠️ +9.17 kB
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..afbf.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
main-2ecb3b8..bd63.js gzip 6.73 kB 6.73 kB
webpack-ccf5..276a.js gzip 751 B 751 B
Overall change 56.8 kB 56.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
main-dbff266..dule.js gzip 5.81 kB 5.81 kB
webpack-10c7..dule.js gzip 751 B 751 B
Overall change 51.8 kB 51.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
polyfills-75..1629.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB
hooks-585f07..95a3.js gzip 887 B 887 B
index-c7b63f..fc02.js gzip 227 B 227 B
link-e97733c..af21.js gzip 1.3 kB 1.3 kB
routerDirect..ebc7.js gzip 284 B 284 B
withRouter-2..db68.js gzip 284 B 284 B
Overall change 7.72 kB 7.72 kB
Client Pages Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-97e743e..dule.js gzip 626 B 626 B
_error-b4004..dule.js gzip 2.3 kB 2.3 kB
hooks-696209..dule.js gzip 387 B 387 B
index-a4dd74..dule.js gzip 226 B 226 B
link-fbb1d2e..dule.js gzip 1.25 kB 1.25 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-1..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_buildManifest.js gzip 272 B 272 B
_buildManife..dule.js gzip 280 B 280 B
Overall change 552 B 552 B
Serverless bundles Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_error.js 1.02 MB 1.02 MB ⚠️ +2.24 kB
404.html 4.18 kB 4.18 kB
hooks.html 3.82 kB 3.82 kB
index.js 1.02 MB 1.02 MB ⚠️ +2.24 kB
link.js 1.06 MB 1.06 MB ⚠️ +2.24 kB
routerDirect.js 1.05 MB 1.05 MB ⚠️ +2.24 kB
withRouter.js 1.05 MB 1.05 MB ⚠️ +2.24 kB
Overall change 5.2 MB 5.21 MB ⚠️ +11.2 kB
Commit: 3ec13d9

@ijjk
Copy link
Member

ijjk commented Aug 5, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
buildDuration 14.2s 14.2s -2ms
nodeModulesSize 66 MB 66 MB ⚠️ +9.17 kB
Page Load Tests Overall increase ✓
vercel/next.js canary azukaru/next.js add-image-postprocess Change
/ failed reqs 0 0
/ total time (seconds) 2.708 2.6 -0.11
/ avg req/sec 923.3 961.36 +38.06
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.68 1.564 -0.12
/error-in-render avg req/sec 1488.53 1598.32 +109.79
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..afbf.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
main-76d73eb..95d8.js gzip 6.73 kB 6.73 kB
webpack-ccf5..276a.js gzip 751 B 751 B
Overall change 56.8 kB 56.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
main-0b6ca7d..dule.js gzip 5.81 kB 5.81 kB
webpack-10c7..dule.js gzip 751 B 751 B
Overall change 51.8 kB 51.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
polyfills-75..1629.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB
hooks-585f07..95a3.js gzip 887 B 887 B
index-c7b63f..fc02.js gzip 227 B 227 B
link-e97733c..af21.js gzip 1.3 kB 1.3 kB
routerDirect..ebc7.js gzip 284 B 284 B
withRouter-2..db68.js gzip 284 B 284 B
Overall change 7.72 kB 7.72 kB
Client Pages Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-97e743e..dule.js gzip 626 B 626 B
_error-b4004..dule.js gzip 2.3 kB 2.3 kB
hooks-696209..dule.js gzip 387 B 387 B
index-a4dd74..dule.js gzip 226 B 226 B
link-fbb1d2e..dule.js gzip 1.25 kB 1.25 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-1..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_buildManifest.js gzip 272 B 272 B
_buildManife..dule.js gzip 280 B 280 B
Overall change 552 B 552 B
Rendered Page Sizes
vercel/next.js canary azukaru/next.js add-image-postprocess Change
index.html gzip 945 B 945 B
link.html gzip 953 B 953 B
withRouter.html gzip 939 B 939 B
Overall change 2.84 kB 2.84 kB

Serverless Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
buildDuration 15.7s 15.5s -242ms
nodeModulesSize 66 MB 66 MB ⚠️ +9.17 kB
Client Bundles (main, webpack, commons)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..afbf.js gzip 10.2 kB 10.2 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
main-76d73eb..95d8.js gzip 6.73 kB 6.73 kB
webpack-ccf5..276a.js gzip 751 B 751 B
Overall change 56.8 kB 56.8 kB
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
677f882d2ed8..dule.js gzip 6.11 kB 6.11 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
main-0b6ca7d..dule.js gzip 5.81 kB 5.81 kB
webpack-10c7..dule.js gzip 751 B 751 B
Overall change 51.8 kB 51.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary azukaru/next.js add-image-postprocess Change
polyfills-75..1629.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB
hooks-585f07..95a3.js gzip 887 B 887 B
index-c7b63f..fc02.js gzip 227 B 227 B
link-e97733c..af21.js gzip 1.3 kB 1.3 kB
routerDirect..ebc7.js gzip 284 B 284 B
withRouter-2..db68.js gzip 284 B 284 B
Overall change 7.72 kB 7.72 kB
Client Pages Modern
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_app-97e743e..dule.js gzip 626 B 626 B
_error-b4004..dule.js gzip 2.3 kB 2.3 kB
hooks-696209..dule.js gzip 387 B 387 B
index-a4dd74..dule.js gzip 226 B 226 B
link-fbb1d2e..dule.js gzip 1.25 kB 1.25 kB
routerDirect..dule.js gzip 284 B 284 B
withRouter-1..dule.js gzip 282 B 282 B
Overall change 5.36 kB 5.36 kB
Client Build Manifests
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_buildManifest.js gzip 272 B 272 B
_buildManife..dule.js gzip 280 B 280 B
Overall change 552 B 552 B
Serverless bundles Overall increase ⚠️
vercel/next.js canary azukaru/next.js add-image-postprocess Change
_error.js 1.02 MB 1.02 MB ⚠️ +2.24 kB
404.html 4.18 kB 4.18 kB
hooks.html 3.82 kB 3.82 kB
index.js 1.02 MB 1.02 MB ⚠️ +2.24 kB
link.js 1.06 MB 1.06 MB ⚠️ +2.24 kB
routerDirect.js 1.05 MB 1.05 MB ⚠️ +2.24 kB
withRouter.js 1.05 MB 1.05 MB ⚠️ +2.24 kB
Overall change 5.2 MB 5.21 MB ⚠️ +11.2 kB
Commit: 282fd13

@kodiakhq kodiakhq bot merged commit b6060fa into vercel:canary Aug 5, 2020
Copy link
Contributor

@prateekbh prateekbh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hunch that this might have missed addressing the serverless case. Sorry for late review

if (isImgEligible(imgElements[i])) {
eligibleImages.push(imgElements[i])
}
if (eligibleImages.length >= MAXIMUM_IMAGE_PRELOADS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dumb/not a very relevant question: does a 5*500 image qualify? If yes, would it be worth preloading?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 by 500 would (just) not qualify for preload since the logic down at line 207 checks to see if width * height is less than or equal to 2500 and disqualifies if that's so. That's pretty arbitrary, though--do you think a different cutoff would be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry I mis-read this line. This makes sense. I feel its ok to start with arbitrary number and change it with some experiment results

)
await nextBuild(appDir)
appPort = await findPort()
app = await nextStart(appDir, appPort)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Timer Timer deleted the add-image-postprocess branch August 5, 2020 18:04
Comment on lines 7784 to 7794
resolved "https://registry.yarnpkg.com/he/-/he-1.1.1.tgz#93410fd21b009735151f8868c2f271f3427e23fd"
integrity sha1-k0EP0hsAlzUVH4howvJx80J+I/0=

[email protected]:
version "1.1.1"
resolved "https://registry.yarnpkg.com/he/-/he-1.1.1.tgz#93410fd21b009735151f8868c2f271f3427e23fd"
integrity sha1-k0EP0hsAlzUVH4howvJx80J+I/0=

header-case@^1.0.0:
version "1.0.1"
resolved "https://registry.yarnpkg.com/header-case/-/header-case-1.0.1.tgz#9535973197c144b09613cd65d317ef19963bd02d"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the significance of this change to yarn.lock?
cc @Timer

@jecassis
Copy link

jecassis commented Aug 6, 2020

The regular expression seems to break with inlined images when scanning for already preloaded ones. Error message:

SyntaxError: Invalid regular expression: /<link[^>]*href[^>]*data:image/png;base64,iVBORw...YII=/: Nothing to repeat

@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants