Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

prioritize_critical_css 5 minutes clarification #1574

Closed
brianbest opened this issue Jun 27, 2018 · 17 comments
Closed

prioritize_critical_css 5 minutes clarification #1574

brianbest opened this issue Jun 27, 2018 · 17 comments

Comments

@brianbest
Copy link

Hey all, I'm looking for some information on how prioritize_critical_css works and why. I've seen some other issues (apache/incubator-pagespeed-mod#1767 , apache/incubator-pagespeed-mod#1311) in regards to the time to live for the inline CSS and I'm just trying to wrap my head around this. Is there a reason that the time to live for prioritize_critical_css on pages is set to 5 minutes? Am I losing something by setting that to a longer value?

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

There is https://github.com/apache/incubator-pagespeed-mod/wiki/Design-Doc:-Critical-CSS-Beaconing

Bumping the expiry is fine, I think. What makes sense probably depends on how dynamic the site is in terms of what is critical to rendering. The default of 5 minutes seems pretty conservative.

@brianbest
Copy link
Author

Thanks for the quick reply @oschaaf. Just one more question on this it looks like Pagespeed can detect when the page has changed, why would there need to be this timer as well? Is this to cover changes that come from external sources (ie: API updates)

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

Pagespeed is only able to detect changes when the beacon js is delivered, which in turn only happens on a cold cache or when existing beacon data expires.

It should technically be possible to discover criticalities using alternate strategies that avoid relying on beacons (like running a headless browser server side in pagespeed), but that is not the way it works today.

@njakobsen
Copy link

So changes to an underlying page that happen within the beacon timer do not invalidate the Pagespeed cache? Only after the 5 minutes (or whatever is configured) will Pagespeed notice that the page is changed (or even deleted)?

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

As far as critical css/image data is concerned, I think yes, correct. Other filters use different ways to cope with changes. Deletion of a page isn’t really a concern. But a page that would display wildly different critical content on each load would be a concern / may result in no or suboptimal inlined critical data.

An example of that is a rotating banner rendered server side with random positions of what is first displayed. On the flip side in this situation inlining the right critical images is a lesser concern because one would probably have other more pressing concerns (from a page speed perspective) given such a page.

@njakobsen
Copy link

njakobsen commented Jun 27, 2018

...and after the TTL it always deletes the optimization instead of checking if the underlying page has actually changed first? Or should we be seeing a request from Pagespeed to the app server for the underlying page, and then a response to the browser from Pagespeed that includes the existing CSS optimization in the case where the underlying page has not changed)?

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

When it is time to re-beacon, pagespeed will inject js into the html to make that happen. So you would be seeing one or more beacon POSTS coming in from browsers on the server, which will be used by PageSpeed to construct a fresh entry into the cache.

In the period where the cache entry is stale and beacons are needed to determine a steady state for on-page critical data, it is possible that no critical css and/or images are inlined.

On site with some traffic, this isn't really a concern in practice, but on a low traffic site this may become significant.
Is this your concern?

One way to mitigate that would be to configure longer expiry times for the beacon data. Or, alternatively, maybe scripting a headless browser to keep the cache warm would help, if for some reason longer beacon expiry times do not work for you.

@njakobsen
Copy link

We have a mix of high traffic and low traffic pages. We are trying to boost the perceived speed for users, as well as present optimized pages to SEMrush which our client uses to analyze page speed. We had the idea to keep the cache warm with a headless browser so the Beacon JS executes, but we wanted to make sure we understood the mechanism before we did a bunch of work only to realize it would not fix the problem, or even negate the effects of Pagespeed.

The only part of this solution that I see being an issue still is SEMrush analyzing pages that receive low traffic. While we would preheat any page when it is updated, our site is too big to spider all pages continuously within a reasonable Beacon TTL, so eventually some of those pages will start responding with unoptimized pages even though their content has not changed.

@njakobsen
Copy link

What I was hoping to get clarification on was when a Pagespeed cache entry is stale, is there a reason it must serve a page without the optimization even when the underlying page has not changed? Can it not be stale, notice the underlying page has not changed, then mark itself fresh again and serve up an optimized page without the need for reinstrumenting on the next two page loads.

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

I see. One thought I have is that maybe a stale-while-revalidate (like) mechanism would be helpful here. Stale data about critical css/images will still be used while at the same time instrumenting the beacons so it gets updated. This might interact badly with the downstream-caching feature, but that aside, it might be doable to make it work like that.

@njakobsen
Copy link

njakobsen commented Jun 27, 2018

Ha! Posted that 14 seconds before you :)

Oh, actually I see our suggestions are slightly different, but similar in that they would not have a period of time where no optimization is present.

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

The design doc I posted above would probably be worth closely looking at too, but I think a good starting point to evaluate what this would take would be looking at:

PrepareForBeaconInsertionHelper()
https://github.com/apache/incubator-pagespeed-mod/blob/c78d8e85622fe8d451ef29d21f89c68f21067fe6/net/instaweb/rewriter/critical_finder_support_util.cc#L345

And code that calls that, e.g.:
CriticalSelectorFinder::PrepareForBeaconInsertion()
https://github.com/apache/incubator-pagespeed-mod/blob/c78d8e85622fe8d451ef29d21f89c68f21067fe6/net/instaweb/rewriter/critical_selector_finder.cc#L160

@njakobsen
Copy link

njakobsen commented Jun 27, 2018

Will do. Thanks for all your help @oschaaf!

@oschaaf
Copy link
Member

oschaaf commented Jun 27, 2018

No problem. If you intend to have a go at making changes, let me know, I'd be happy to assist / review.

@brianbest
Copy link
Author

Hey @oschaaf im just exparimenting with the beacon and I want to be sure I’m getting this right. If I set the following line in to my pagespeed conf will we only rebeacon every 8000 seconds? would that also keep the beacon data around for 8000 seconds as well? ‘pagespeed BeaconReinstrumentTimeSec 8000;’

@oschaaf
Copy link
Member

oschaaf commented Jun 28, 2018

That sounds right to me, under the precondition that the cache is sized appropriately.
(Else eviction might thwart your plans)

@brianbest
Copy link
Author

Perfect! Yeah, we updated our cache limits to take that into account :). However, we did find out that using the extend cache filter with prioritize_critical_css might have been causing more problems as well. Looks like it will check the page content every 300 seconds or so, and when it does the critical CSS will not show up again until a user visits. We can do without extend cache filter for the time being and removing it fixes it for us but I thought it would be a good idea to mention it to you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants