-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Startup optimizations #226
Conversation
This is looking good. Couple of points: I disagree with what we're doing with the font. The render is now unstable during load, and the instability affects all our areas of first interactivity. I think the best options are sticking with the base64, or removing the font entirely. With the font: Without the font: @developit @kosamari @surma what do you think? Should we just drop the font? We shouldn't render interactive elements that don't yet have behavior attached. We should either ship a very small bit of script that can handle the interaction, but I think it's enough to just hide those elements. I guess I think our pre-JS render should be limited to: |
src/components/intro/style.scss
Outdated
@@ -47,7 +49,7 @@ | |||
} | |||
|
|||
.open-image-guide { | |||
font: 300 11vw intro-text; | |||
font: 300 11vw intro-text,helvetica,arial; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, sans-serif
has the same effect as helvetica,arial
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc just helvetica would as well, since windows substitutes Arial anyway. good catch.
I think we should inline a subset font for the first render, mainly because of two reasons:
|
I'm fine inlining the font or removing it. TBH I'd rather spend those bytes on inlining handlers for drop+click that enqueue to get our TTI down. |
Alright here's the latest result with the fonts inlined and scripts back to |
src/components/intro/style.scss
Outdated
@@ -10,6 +11,7 @@ | |||
font-family: 'intro-text'; | |||
font-style: normal; | |||
font-weight: 500; | |||
font-display: fallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this do anything now we're inlining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data URLs do take a few ms according to lighthouse, but I guess that blocks rendering anyway. Would it make sense to switch this to block
, or just remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah make it block. That covers browsers that might allow a swap (Edge, maybe?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits in the comments, but the main problem is this isn't building. I'm seeing the same problem Netlify is.
ERROR in TypeError: Cannot read property 'readFile' of undefined
- critters.js:97
[squoosh]/[critters-webpack-plugin]/dist/critters.js:97:19
- new Promise
- critters.js:96 Critters.readFile
[squoosh]/[critters-webpack-plugin]/dist/critters.js:96:12
- critters.js:153 Critters.<anonymous>
[squoosh]/[critters-webpack-plugin]/dist/critters.js:153:25
- new Promise
- critters.js:150 Critters.<anonymous>
[squoosh]/[critters-webpack-plugin]/dist/critters.js:150:16
- new Promise
- critters.js:139 Critters.embedLinkedStylesheet
[squoosh]/[critters-webpack-plugin]/dist/critters.js:139:12
- critters.js:114
[squoosh]/[critters-webpack-plugin]/dist/critters.js:114:83
- Array.map
- critters.js:114 Critters.<anonymous>
[squoosh]/[critters-webpack-plugin]/dist/critters.js:114:47
- new Promise
- critters.js:106 Critters.process
[squoosh]/[critters-webpack-plugin]/dist/critters.js:106:12
- critters.js:65
[squoosh]/[critters-webpack-plugin]/dist/critters.js:65:24
- new Promise
- Hook.js:154 AsyncSeriesWaterfallHook.lazyCompileHook
[squoosh]/[tapable]/lib/Hook.js:154:20
- index.js:673
[squoosh]/[html-webpack-plugin]/index.js:673:47
- index.js:196 Promise.resolve.then.then.then.then.then.then.then.then.result
[squoosh]/[html-webpack-plugin]/index.js:196:18
- next_tick.js:68 process._tickCallback
internal/process/next_tick.js:68:7
update: fixed the readfile stuff, but I'm still struggling to get both compilers producing the same contenthashes. Taking forever. |
Root cause turned out to be this bug that was fixed last month. Updating and tweaking a couple things fixed hashes and thus the build: |
@@ -110,7 +110,7 @@ export default class Intro extends Component<Props, State> { | |||
<div class={style.demo}> | |||
<div class={style.demoImgContainer}> | |||
<div class={style.demoImgAspect}> | |||
<img class={style.demoIcon} src={demo.iconUrl} alt=""/> | |||
<img class={style.demoIcon} src={demo.iconUrl} alt="" decoding="async" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakearchibald - I'd thought about having these images get skipped when prerendering. Right now because they're sent as static HTML, they kick of 4 requests that extend our "load" time out by quite a bit (which is more of a metrics issue, but still). Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only metrics I care about are "time until useful pixels are on the screen" and "time until the user can perform the first interaction".
If one of Google's many acronyms reports the wrong time for those, it's their bug, not our's 😄 (we should report it though).
@developit that bug sounded like a nightmare to track down. Thanks for working through it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main.css
contains a lot of duplicated stuff from the inlined CSS. It might be better/simpler to just inline main.css
and not include it as an external file.
Also, the <noscript>
stuff is pointless for us.
@jakearchibald I put some time into improving Critters, and it now strips inlined critical styles out of the lazy-loaded noncritical stylesheets. I also added a threshold option so it doesn't do something silly like lazy-load 300b of CSS and instead just inlines the rest. With those in-place, I think everything is optimal here: Our |
Alright, fairly happy with this now: |
src/index.ts
Outdated
await import('@webcomponents/custom-elements'); | ||
} | ||
|
||
if (!('customElements' in self)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is fragile (as in, it needs to be written in this way else webpack or whatever doesn't do the right thing), add a comment stating that, else I'll probably break it again 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit, but this looks great.
It'll regress a bit when I add CSS to hide non-interactive bits. We may want to adapt our approach a bit to cater for that. We may want to inline the logo, and maybe even the initial bundle
We can experiment more once the UI branches have landed.
In fact, the more I stare at the webpagetest results, we could save half a second by inlining the first-interaction JS. We'll look at that in another branch though. |
324847d
to
58ec1ab
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
I've rebased this onto the service worker branch for now |
58ec1ab
to
b848992
Compare
lol aaaaand back to master |
https://www.webpagetest.org/result/181108_DQ_daaae433b6dd49e4cd0dea126b3e1983/ here's the result of inlining. It seems to regress performance. |
Doesn't actually seem like a regression vs not-inlining https://www.webpagetest.org/result/181108_P7_99f8c7b040cff1b713ce6e11d7efbdde/#run3. Render time stays the same, but it brings interaction time down by a few tenths. |
a91f84b
to
6db451c
Compare
* Startup optimisations * I hate this file * Inline main script * Reverting change to do a fairer perf comparison * Inlining again. Weeeeee! * Lockfile
I played with both to get the HTML payload to 2.27kB.
Also fixes #221.
<script defer>
in head actually performed worse than<script async>
at the end of body with a preload in head, though I didn't test exhaustively (mostly successive LH runs).