Reduce the processing on not-cached page when checking the LCP/ATF data

[piotrbak]

Is your feature request related to a problem? Please describe. On not cached page we're checking if LCP/ATF data exists twice:

1. For the first time when trying to inject the data
2. For the second time when beacon script is triggered

Describe the solution you'd like Instead of checking it for the second time, we could bail-out if we're on not cached page. That's potentially an improvement in terms of resource usage.

Acceptance Criteria

1. When the page is not cached yet, beacon script should not check for existence of the LCP/ATF data in the db
2. When the page is cached, beacon script should check if the data exist
3. If we have data related to this URL (and device), we should bail-out
4. If we don't have data related to this URL (and device), we should proceed

[piotrbak]

This issue was supposed to be part of: https://github.com/wp-media/wp-rocket/issues/6432

But it's not a requirement for initial 3.16.

CC @MathieuLamiot

[Miraeld]

Hello hello,

I get that we want to tackle this issue. I've been brainstorming some solutions and had a chat with @Tabrisrp.

One idea was to check if the visited URL was cached here. The plan was to introduce a new parameter, is_cached, in rocket_data_lcp. This parameter could be used in the beacon itself to bailout before sending the request to the above_the_fold table. But, it turns out this approach has a flaw. If it's the first visit, the page won't be cached, so is_cached would be false, which is good. But, it would also cache the page with is_cached set to false. So, subsequent visits would always bail out. Plus, I realized I hadn't considered all possible file name combinations in the cache folders, which makes things even more complicated.

After chatting with @Tabrisrp, another idea was to do a DB request on the cache table, which can also be filtered by is_mobile. While this sounds good on paper, it doesn't really solve the problem. The main goal is to avoid unnecessary DB requests, but this new solution just swaps one DB request for another. Whether we query the cache table or the above_the_fold table, it ultimately adds another request. Also, it can even add a request, which I guess we don't want, in the following scenario: When we load the page, we would check cache table, if it is cached, we will check above_the_fold for data, so it's even adding another request.

So, I'm a bit stumped on how to approach this issue. If anyone has an idea, please feel free. @piotrbak @MathieuLamiot

[MathieuLamiot]

Indeed, checking the DB to know if the page is cached won't help here to reduce processing. What I had in mind was to do the same way WPR Inspector does it here

It seems pages served from the cache have this at the end in the HTML: Debug: cached@1714719972 -->

[Miraeld]

This is only valid if the page gets cached with this in the wp-config: define( 'WP_ROCKET_DEBUG', true );

EDIT: Maybe I was wrong, did a test that works without that constant. Let me check the code.

[Miraeld]

Scope a solution

First of all, we need to detect when the page is cached or not. To do that, within our class in lcp-beacon.js we need to add a new function:

	_isPageCached() {
		let signature = '';
		let status = false;
		if (document.documentElement.nextSibling)  {
			signature = document.documentElement.nextSibling.data;
		}
		if ( signature && signature.includes( 'Debug: cached' ) ) {
			return true;
		}
		return false;
	}

This function will check the signature from WP Rocket to see if there is the part Debug: cache which is added to the end of the signature when the page is cached.

Once done, we need to modify the function _isValidPreconditions() to add the check if the page is cached before sending a DB request through the AJAX call:

		if ( this._isPageCached() ) {
			if ( this._isGeneratedBefore() ) {
				this._logMessage('Bailing out because data is already available');
				return false;
			}

And finally, as I noticed while grooming that _isGeneratedBefore() is currently always returning undefined we need to modify it to fix it:

	async _isGeneratedBefore() {
		// AJAX call to check if there are any records for the current URL.
		let lcp_data_response;
		let data_check = new FormData();
		data_check.append('action', 'rocket_check_lcp');
		data_check.append('rocket_lcp_nonce', this.config.nonce);
		data_check.append('url', this.config.url);
		data_check.append('is_mobile', this.config.is_mobile);

		await fetch(this.config.ajax_url, {
			method: "POST",
			credentials: 'same-origin',
			body: data_check
		})
		.then(data => {
			lcp_data_response = data
		});

		return lcp_data_response.success;
	}

Efforts:

S

[MathieuLamiot]

Just a though crossing my mind reading this: If we start using the signature as a "feature", maybe it'd be good to change the wording to something else than Debug?

[Mai-Saad]

@MathieuLamiot @piotrbak What will happen with the white label used? https://docs.wp-rocket.me/article/7-enabling-white-label

[MathieuLamiot]

We should handle it as well, as I mentioned here: https://github.com/wp-media/wp-rocket/pull/6610#issuecomment-2107878098

But the PR lacks follow-up, we discussed opening a follow-up issue for the discussed enhancement (possibly including the white label handling) a few days ago already. @wp-media/engineering-plugin-team can you make sure to capture the leftover discussed in the PR to a dedicated GH issue so that the scope of this PR vs. Leftover is clear and we can close it.

[wordpressfan]

Going to create a github issue for this now

[wordpressfan]

I created the issue here: https://github.com/wp-media/wp-rocket/issues/6637 If u have any more input plz add it there.

For the whitelabel point:

When the constant WP_ROCKET_WHITE_LABEL_FOOTPRINT is there we still add the Debug: cached here and we search for it in the new beacon here So we are fine here

[MathieuLamiot]

Thanks boss!

[MathieuLamiot]

Async/Await issues: the fetch method is async so we can't use it in a sync process to check an HTTP response. Discussed here Moving this back to In progress.