wp-rocket icon indicating copy to clipboard operation
wp-rocket copied to clipboard

3.17: All hashes are saved to DB for LRC

Open Mai-Saad opened this issue 1 year ago • 4 comments

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version => commit https://github.com/wp-media/wp-rocket/commit/b34405a0338f91b1fba488b034f61be4822b348f https://github.com/wp-media/wp-rocket/pull/6888
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug When visiting the uncached nor warmed-up page, all hashes are added to the DB LRC table instead of adding only eligible ones in the threshold

To Reproduce Steps to reproduce the behavior:

  1. Visit the uncached page (has no lcp nor lrc yet)
  2. Check the LRC table

Expected behavior A clear and concise description of what you expected to happen.

  • only eligible hashes are added ( the ones above the 1800 threshold)
  • For elements within the threshold, parents hash is enough

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

Acceptance Criteria (for WP Media team use only) Clear instructions for developers, to be added before the grooming

  • Page with all elements above the fold => no hashes in DB
  • Page with some elements in threshold => hashes added in DB
  • Page with big HTML is handled => related https://wp-media.slack.com/archives/CUT7FLHF1/p1724324937592149?thread_ts=1724257215.870589&cid=CUT7FLHF1
  • No error for pages with no HTML closure, no body closure , empty HTML => related thread https://wp-media.slack.com/archives/CUKB44GNN/p1720610430651959?thread_ts=1720196732.934469&cid=CUKB44GNN

Mai-Saad avatar Aug 22 '24 14:08 Mai-Saad

@Mai-Saad, about this:

Page with big HTML is handled => related https://wp-media.slack.com/archives/CUT7FLHF1/p1724324937592149?thread_ts=1724257215.870589&cid=CUT7FLHF1

You are seeing the hashes added to the DB, but there is a failed status with timeout message, correct?

Maybe this is expected because the beacon took too long to run? IT could be worth checking by increasing the allowed time (there is a filter for this). Unless @jeawhanlee identified a bug explaining this?

MathieuLamiot avatar Aug 22 '24 15:08 MathieuLamiot

@MathieuLamiot with the latest feature/lrc (commit fb50ef3b9db98b09b4b90797f5c16117a1560435 ) can see hashes added fine without using delay filter, however, after clear cache and visit the page, we will have this warning [26-Aug-2024 10:10:22 UTC] PHP Warning: preg_replace(): Compilation failed: regular expression is too large at offset 3296566 in /var/www/newer.rocketlabsqa.ovh/htdocs/wp-content/plugins/wp-rocket/inc/Engine/Optimization/LazyRenderContent/Frontend/Controller.php on line 55 thus style won't be added to the page ==> in this case script is removed from the page but hashes are still applied (same will happen even if with delay of 20s )

add_filter( 'rocket_performance_hints_optimization_delay', function( $delay ){
    return 20000;
});

Mai-Saad avatar Aug 26 '24 11:08 Mai-Saad

Oh interesting. I suppose there are many hashes in the LRC table for this URL? PHP is complaining that the pattern we are looking for is too large here, likely because the resulting string out of implode( '|', $hashes ) is way too long. I would consider this an edge case, that could be dealt with in a dedicated GH issue, likely not mandatory for the release 🤔 WDYT @wp-media/product ?

MathieuLamiot avatar Aug 26 '24 11:08 MathieuLamiot

What was tested is quite large and won't fit on the normal use for most customers.

3296566 is actually quite advanced and is 3x the default PCRE limit 1000000.

Support can temporarily attempt to increase the limit on PHP if needed:

ini_set('pcre.backtrack_limit', '4000000');
ini_set('pcre.recursion_limit', '400000');

I don't expect this to an issue in normal conditions.

Edge case.

Not mandatory to deal with on release.

DahmaniAdame avatar Aug 26 '24 12:08 DahmaniAdame