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

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

Closed
piotrbak opened this issue Apr 18, 2024 · 12 comments · Fixed by #6610
Closed

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

piotrbak opened this issue Apr 18, 2024 · 12 comments · Fixed by #6610
Assignees
Labels
lcp needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

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 piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: medium Issues which are important, but no one will go out of business. needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. lcp labels Apr 18, 2024
@piotrbak
Copy link
Contributor Author

This issue was supposed to be part of:
#6432

But it's not a requirement for initial 3.16.

CC @MathieuLamiot

@Miraeld
Copy link
Contributor

Miraeld commented May 2, 2024

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
Copy link
Contributor

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
Copy link
Contributor

Miraeld commented May 3, 2024

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
Copy link
Contributor

Miraeld commented May 3, 2024

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
Copy link
Contributor

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?

@piotrbak piotrbak added this to the 3.16.1 milestone May 16, 2024
@Mai-Saad
Copy link

Mai-Saad commented May 17, 2024

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

@MathieuLamiot
Copy link
Contributor

We should handle it as well, as I mentioned here: #6610 (comment)

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
Copy link
Contributor

Going to create a github issue for this now

@Mai-Saad Mai-Saad self-assigned this May 17, 2024
@wordpressfan
Copy link
Contributor

I created the issue here: #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
Copy link
Contributor

Thanks boss!

@MathieuLamiot
Copy link
Contributor

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.

@wordpressfan wordpressfan self-assigned this May 23, 2024
github-merge-queue bot pushed a commit that referenced this issue May 23, 2024
…the LCP/ATF data (#6610)

Co-authored-by: Opeyemi Ibrahim <opeyemi.khadri@gmail.com>
Co-authored-by: WordPressFan <ahmed@wp-media.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lcp needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
5 participants