angular / universal

Server-side rendering and Prerendering for Angular
MIT License
4.04k stars 484 forks source link

inlineCriticalCss is slow #2106

Closed amakhrov closed 1 year ago

amakhrov commented 3 years ago

🐞 Bug report

What modules are related to this issue?

Is this a regression?

No

Description

Recently I tried to enable inlineCriticalCss on our project. And had to roll it back quickly - SSR times jumped sky high.

styles.css in our project is around 250KB. This includes Angular Material (core + theme) and a number of old-style components that have not been converted to styleUrls yet. However even if I strip it down to bare minumum - it's still ~100KB (majority of that are Material styles).

HTML is also somewhat significant - 200KB (after I stripped it off of transfered state), and contain 1500 DOM nodes.

Profiling shows that most of the css inlining time is spent on findOne call.

My understanding is that critters has complexity roughly O(N * S), where N is number of DOM nodes, and S is number of selectors in the stylesheets. That's because it queries every selector against the DOM. And our stylesheet (the minimal version) has 1000+ selectors. The regular prod version of the stylesheet - 2600 selectors.

I'm wondering if it could be improved by grouping selectors and discarding a whole group if the top-most selector was not found. E.g if we checked .mat-button selector and didn't find any matching node - there is no point in looking for more specific selectors like .mat-button .mat-button-focus-overlay

πŸ”¬ Minimal Reproduction

🌍 Your Environment


Angular CLI: 11.2.12
Node: 12.14.1
OS: darwin x64

Angular: 11.2.13
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... platform-server, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1102.12
@angular-devkit/build-angular   0.1102.12
@angular-devkit/core            11.2.12
@angular-devkit/schematics      11.2.12
@angular/cdk                    11.2.12
@angular/cli                    11.2.12
@angular/material               11.2.12
@nguniversal/builders           11.2.1
@nguniversal/express-engine     11.2.1
@schematics/angular             11.2.12
@schematics/update              0.1102.12
rxjs                            6.6.3
typescript                      4.1.5
alan-agius4 commented 3 years ago

Hi @amakhrov,

Thanks for trying the feedback. I'll pass this to the Chrome SDK team since they are responsible of critters.

alan-agius4 commented 3 years ago

@janicklas-ralph kindly see above.

webcat12345 commented 3 years ago

Thank you very much @alan-agius4 I will watch this issue from now on. :)

webcat12345 commented 3 years ago

Just sharing my case, not just slow, it use the CPU more than 100% and server goes down and it was really critical.

https://github.com/angular/angular/issues/42098

amakhrov commented 3 years ago

I did existing tools research and wanted to share some thoughts here. Most tool work similarly to Critters - they individually test each selector against full dom.

However, PurgeCSS (https://github.com/FullHuman/purgecss) uses a different approach: it extracts all class names, tag names, ids and attributes from html, stores them in sets. And then checks whether all parts of a particular css selector are included in those sets. Since set lookup is constant, it gives a solid CPU boost. The tradeoff is that the result is less precise: some complex CSS rules are incorrectly marked as "used", while they do not match the actual DOM tree shape.

Critters makes a lot of sense with pre-rendering. However, SSR suffers a lot from its CPU-bound computations. Maybe we could use different tools for those two use cases? Less precise but faster - for SSR. And more precise but slower for pre-rendering.

ubaidazad commented 3 years ago

Even disabling inlineCritical in angular.json do not solve this problem

"optimization": {
  "scripts": true,
  "fonts": true,
  "styles": {
    "minify": true,
    "inlineCritical": false
  }
}

also in server block

"optimization": {
  "scripts": true,
  "fonts": false,
  "styles":  false
}

How can inlineCritical be disabled and not affecting SSR and CPU utilization?

Angular CLI: 12.0.4
Node: 14.17.0
alan-agius4 commented 3 years ago

You need to disable it in your server.ts.

https://github.com/angular/angular/issues/42098#issuecomment-841629552

ubaidazad commented 3 years ago

Thanks @alan-agius4 this actually helped

in server.ts

server.engine('html', ngExpressEngine({
  inlineCriticalCss: false,
  bootstrap: AppServerModule,
}));
kalebdf commented 3 years ago

Thank you @alan-agius4 & @ubaidazad. πŸ’― πŸ‘ Temporarily disabling the feature in server.ts with inlineCriticalCss: false worked for us. We await future speed improvements.

At FloSports, we have a content-rich web app and several news articles with chunky HTML tables (~150KB) of athlete/team data (imagine an Excel dumps worth). Those articles really magnified the speed issue with +20s renders. In addition to these articles, in general, we saw an increase in SSR latency of 2.1x to 3.2x across all of our pages.

We didn't notice the issue until it deployed to production. Below you can see when we were alerted via Datadog of an anomaly and debugged a bit before manually rolling back the deployment.

image

peturh commented 2 years ago

Same happens for us. We've seen insane response time peaks and not inlining css is not an option for us. Is only option downgrade or is this fixed in 13?

2021-11-18-145835_1624x896_scrot

alan-agius4 commented 2 years ago

@peturh, by downgrading you will be disabling critical css inlining. It is also unclear which version you are using, there were some changes in critters recently and are available in v13+ and the latest patch version of v12, which is 12.1.3.

peturh commented 2 years ago

We've used: Angular ~12.2.13 and nguniversal 12.1.3

"dependencies": {
    "@angular/animations": "~12.2.13",
    "@angular/cdk": "^12.1.1",
    "@angular/common": "~12.2.13",
    "@angular/compiler": "~12.2.13",
    "@angular/core": "~12.2.13",
    "@angular/forms": "~12.2.13",
    "@angular/platform-browser": "~12.2.13",
    "@angular/platform-browser-dynamic": "~12.2.13",
    "@angular/platform-server": "~12.2.13",
    "@angular/router": "~12.2.13",
    "@angular/service-worker": "~12.2.13",
    "@microsoft/applicationinsights-web": "^2.7.0",
    "@ngneat/until-destroy": "^8.1.4",
    "@nguniversal/express-engine": "^12.1.3",
    "@ngx-meta/core": "^9.0.0",
    "@ngx-translate/core": "^13.0.0",
    "@ngx-translate/http-loader": "^7.0.0",
    "dotenv": "^8.2.0",
    "express": "^4.17.1",
    "inversify": "^6.0.1",
    "inversify-express-utils": "^6.4.3",
    "ngx-cookie-service": "^11.0.2",
    "ngx-infinite-scroll": "^10.0.1",
    "node-cache": "^5.1.2",
    "reflect-metadata": "^0.1.13",
    "rxjs": "~6.6.3",
    "tslib": "^2.1.0",
    "webpack-visualizer-plugin": "^0.1.11",
    "zone.js": "~0.11.4"
  },
  "devDependencies": {
    "@angular-devkit/architect": "^0.1202.13",
    "@angular-devkit/build-angular": "~12.2.13",
    "@angular-devkit/schematics": "^12.2.13",
    "@angular/cli": "~12.2.13",
    "@angular/compiler-cli": "~12.2.13",
    "@nguniversal/builders": "^12.1.3",
    "@types/cookie-parser": "^1.4.2",
    "@types/express": "4.17.8",
    "@types/jasmine": "~3.6.3",
    "@types/jasminewd2": "~2.0.8",
    "@types/node": "^16.11.7",
    "cypress": "^8.7.0",
    "prettier": "2.2.1",
    "ts-node": "~9.1.1",
    "typescript": "~4.3.5",
  },
alan-agius4 commented 2 years ago

That cannot be right, because @nguniversal packages were not released as 12.2.x.

peturh commented 2 years ago

That cannot be right, because @nguniversal packages were not released as 12.2.x.

Sorry, updated my post with my current package.json

peturh commented 2 years ago

@alan-agius4

So after doing this correctly and using correct packages, I still suffer a lot in response time on the server. With the setting inlineCriticalCss: false it is now good. However, the extracted CSS that we had before in version 11 is now gone and the page moves in a bad way on load without some css. What is the suggested solution for this now when extractCss is deprecated andextractCriticalCss` isn't working?

Update:

We added our global css to app.component.less and set encapsulation: ViewEncapsulation.None, in the app.component.ts

Update 2:

Followed @alan-agius4 example and added:

"optimization": {
    "styles": {
         "inlineCritical": false
    }
},

to angular.json browser build.

alan-agius4 commented 2 years ago

If you disable inlineCriticalCss on the server, make sure you also disable this in the browser build by changing the optimization option in angular.json

See : https://angular.io/guide/workspace-config#optimization-configuration.

CarlosTorrecillas commented 2 years ago

I have been working on improving the lighthouse score in one my sites and I upgraded to Angular 13 recently. I read this thread and removing inlineCriticalCss from the server.ts file and setting the build with the optimize: true flag worked like a charm. Site performance / score went up big time

poonga commented 2 years ago

@CarlosTorrecillas Hi, Did you face any flicker issues in Angular 13? I meant the bootstrap loading delayed for the first-page load. On first page load the UI was broken then back to normal fraction of seconds

klemenoslaj commented 2 years ago

As long as the HTML generated by SSR is identical to the client HTML there should be no flickering - or at least I don't experience it.

Try refreshing the app with JavaScript disabled and observe what's wrong with the HTML.


What I do experience is scrollbar jumping like there's no tomorrow, when you refresh the page with scrollbar not at the top - it jumps to top on refresh and back to original position when angular bootstraps. However the HTML is not broken.

CarlosTorrecillas commented 2 years ago

@poonga in my case I did have very little flickering. Still present but sometimes you don't even notice. For me the issue was the lighthouse ranking in the page speed score for Google which was improved

poonga commented 2 years ago

@CarlosTorrecillas Could you please suggest some tips to address those feedbacks? image

CarlosTorrecillas commented 2 years ago

Hi @poonga ,

Without looking at the code / setup, I would recommend several checks to try mitigating the errors / bad score here:

  1. Reduce initial server response time: 1) Take a look at your server, check for high CPU / MEM processes to see if there is anything obvious or something you can improve. 2) Check if your initial bootstrap of your application is inefficient. Make sure you use lazy loading and also you only load the components you need when loading the app. 3) Check long running API calls that may be affecting this initial load.

  2. Reduce unused javascript: Check your bundle size and whether you are building them correctly. To be honest, most of my apps are now complaining about it and I have been optimizing code for a while without much success.

  3. Serve images in next gen format: Make sure your images have good format such as webp and also have the right size and have defined the height and width accordingly in you style or html.

  4. Efficiently encode images. Never came across that one.

  5. Ensure text remains visible during webfont load: In recents page speed tests of my apps that had to do with the way of loading Google fonts. I used to have webfontloader (JS) however I switched to do it via <link and also doing preconnects to the Google servers. There is a lot of examples on the web.

  6. Does not use passive listeners to improve scrolling performance. Never came across that one before but I guess you have hooks somewhere that are slowing down the app when you scroll / mouse wheel?

  7. Serve static assets with an efficient cache policy: 1) Make sure your web server is adding the cache header to the images you are serving so that they get cached in the client. That will depends on the type of web server for the specific setup but there is a lot of documentation for all available servers out there. 2) If you're using a CDN make sure you have the cache policy correctly set up.

  8. Avoid an excessive DOM size: Check if you can optimize your components so that the HTML can be reduced. As a good practice I tend to split my pages into really tiny components so that I can run checks on that quite easily. Good parts to check are ngFors where you create many elements. If that was not something you could reduce, at least try to apply above the fold / uner the fold techniques to reduce the amount of elements loaded at the bootstrap of the app. That way you will be adding to the DOM progressively which is more performant.

  9. Minimize main thread work: Again, check the overall workflow of the application. Check listeners, timeouts, api calls, etc. Try to see if you can optimize or make some concurrent calls, reduce listeners or implement them in a different way. If you're using SSR which I believe you do, make sure you don't run unnecessary code in your server side, such as animations, times, or simply rendering code that you may not need

Hope this helps. This is my two cents based on my experience with Angular SSR.

vytautas-pranskunas- commented 2 years ago

watching

sir-captainmorgan21 commented 2 years ago

@CarlosTorrecillas how did this not cause your site to flicker as the browser waits for your CSS file to be loaded?

Also, does anyone know if inlineCriticalCSS is defaulted to true or false?

sir-captainmorgan21 commented 2 years ago

@alan-agius4 other than turning everything off, is there any progress on this?

hiepxanh commented 2 years ago

@sir-captainmorgan21 https://github.com/angular/preboot/issues/135#issuecomment-1081950340 here you are bro no flickering

CarlosTorrecillas commented 1 year ago

In order to avoid any confusion, guys could you please clarify the way of fully disabling the inlining of critical css? I say that because I don't whether setting in the server.ts is "just" enough or you have also to configure it in the angular.json. Would be nice to know all sections related that need that.

Many thanks!

brjadams commented 1 year ago

When i went from angular 11 -> 12 there was a huge performance hit due to critical css and I had to disable like shown here.

Looking to go upgrade a handful of applications to Angular 15, and if performance is still an issue?

adzza24 commented 1 year ago

I'm on Angular 14 and having the same performance lag. With inlineCriticalCSS: true the app takes >5s to respond to a GET request. Even with server-side caching it still takes 3s. With inlineCriticalCSS: false it responds in ~200ms.

tomazn commented 1 year ago

I'm on Angular 14 and having the same performance lag. With inlineCriticalCSS: true the app takes >5s to respond to a GET request. Even with server-side caching it still takes 3s. With inlineCriticalCSS: false it responds in ~200ms.

I have the same performance improvement with inlineCriticalCSS: false, unfortunately when opening a new tab I have a blink of unstyled html. Does anyone else have this problem? Is there any workaround ?

smrb76 commented 1 year ago

Isn`t fix in angular v15?

phihochzwei commented 1 year ago

Isn`t fix in angular v15?

Nope, unfortunately not. I am on NG 15.1 inlinceCSS on -> 5.6-8.9s inlinceCSS off -> 0.5-1.1s

amcgranemyhome commented 1 year ago

If you disable inlineCriticalCss on the server, make sure you also disable this in the browser build by changing the optimization option in angular.json

See : https://angular.io/guide/workspace-config#optimization-configuration.

For anyone having the flickering when they set inlineCriticalCSS: false in server.ts, just do this to fix it:

"optimization": { "scripts": true, "styles": { "minify": true, "inlineCritical": false }, "fonts": true }

CarlosTorrecillas commented 1 year ago

Do we still have to set "inlineCritical": false in the server.ts to get the best performance possible? I can see that's not present in the .zip for the universal example in angular.io and I don't know if that example is fully up to date. Could you confirm @alan-agius4 ? Thanks!

dave-vdg commented 1 year ago

Setting "inlineCritical": false speeds up our app by aprox. 30%. Nevertheless this brings the CLS value up to nearly 1 (no bueno). Is there anything else we can do to fix this issue asap?

CarlosTorrecillas commented 1 year ago

@dave-vdg in my case, setting "inlineCritical": true (I am not setting it so the default value applies) is making my site performing worse. I still need to use it on version 15.2.5 because I am facing a penalty of nearly 9% in performance for desktop view and 2-3% on mobile. In terms of Lighthouse evaluation I go from 0.001 to 0.17 CLS (desktop).

I don't know if that is going to get fixed in Angular 16 but in the meantime, if I were you I would try to search for optimizations you can do yourself because that's what's mostly is going to improve your site performance.

peturh commented 1 year ago

The problem for us has been the "large" amounts of included CSS. If we ignore the third party css that we're using (in our case uikit) this problem disappears. Right now it feels like this will never be fixed.

It feels like the Angular team wants us to clean up our CSS imports and use a better CSS framework (tailwind works fine for example). Lots of apps on the web doesn't today however.

CarlosTorrecillas commented 1 year ago

@peturh are you doing any kind of PurgeCSS to try mitigating this problem? Or is it that you use a lot of CSS of that library?

peturh commented 1 year ago

@CarlosTorrecillas We never got it to work in production, we rely to much on third party CSS to do so. We just removed the CSS imports to see if it worked, and it did.

CarlosTorrecillas commented 1 year ago

Do you know your production style.css weight? Say that just in case you can get around it by moving your library away from being imported in your styles.css (assuming you are doing so) and get all the imports on a different css that you load with priority. Then, of course, applying CSS minification and cleaning up unused (I am assuming you are doing that as well). This post is a very good example that shows you how to improve performance in case that is of any use: https://dev.to/brihoum/how-i-optimized-my-angular-website-3fd0

alan-agius4 commented 1 year ago

In version 16.1.0-rc.0 we have updated critters which contains several performance improvements.

CarlosTorrecillas commented 1 year ago

Hopefully @alan-agius4 you guys get that stable real soon so we can begin the upgrades!

alan-agius4 commented 1 year ago

Hopefully @alan-agius4 you guys get that stable real soon so we can begin the upgrades!

The stable release is scheduled to happen next week.

brjadams commented 1 year ago

@alan-agius4 - will this fix be back ported to 15.x as it is LTS?

alan-agius4 commented 1 year ago

@brjadams, no, please see the LTS policy.

CarlosTorrecillas commented 1 year ago

@alan-agius4 do you think after we upgrade to v16.1.0 we can set inlineCriticalCss to true on both the browser and the server or do you guys expect to have further performance improvements?

alan-agius4 commented 1 year ago

@CarlosTorrecillas, there are no further performance improvements planned for critical CSS inlining at the moment.

CarlosTorrecillas commented 1 year ago

OK, would you be able to confirm whether reverting back to "inlineCriticalCss: true" is, as performant (or even more performant) than the current workaround where we had to disable that optimisation on both the browser and the server? Essentially I want to know if it is safe for us to put back that setting or not. Not sure if you guys had any kind of benchmark around that area that can throw out some numbers to reflect it? Thanks!

alan-agius4 commented 1 year ago

Enabling inlineCriticalCss does impact the rendering times. How much of an impact it has varies based on the amount of CSS, CSS selector complexity and DOM size a page has.

That said, this should only effect the first request for every given page if the response is being cached at a CDN/server cache layer. Caching is highly advised regardless of enabling inline critical CSS or not.

Overall, I recommend to benchmark your application and would appreciate if you could share some numbers with us.

CarlosTorrecillas commented 1 year ago

@alan-agius4, I think I can have a baseline for the last 90 days because I can access the Google Search Console report for the crawling stats. Assuming this can be accepted as a valid benchmark I can see the impact of having the flag turned ON in April where the AVG response time went over 400ms peeking 470 and I disabled (inlineCriticalCss: false on both server and browser since the begging of June where you can see the improvement).

In my case, the static content gets cached in the browser via headers and the dynamic content that gets generated by APIs is automatically cached on the server. At the moment I can see response times that to me are "OK / GOOD" - 270ms aprox. since last week or so when I re-disabled the flag.

I plan to upgrade to Angular 16.1.1 next Monday and will start collecting data so I can share. In my scenario I will be using provideClientHydration and will be disabling (ngSkipHydration) on a couple of components due to errors rending them - outerHTML tag that I noticed and others that still don't really know why are triggered - I made an attempt yesterday of deploying v16.0.5 after zone.js was updated to 0.13.1 and I was getting slightly worse performance according to the Lighthouse - note that inlineCriticalCsswas still false, so I guess it would be good to get visibility about the performance impact of the ngSkipHydration- if any - and also whether the latest Angular version will perform better with inlineCriticalCss : true (so optimization: true). Does that make sense?

I can share with you, at least, the graph I just collected from Google Search Console about my site where you can clearly see the performance hit during the past two months or so when I put inlineCriticalCssenabled.

image