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

Improvements to homepage fetching process #6613

Closed
piotrbak opened this issue May 3, 2024 · 1 comment · Fixed by #6615
Closed

Improvements to homepage fetching process #6613

piotrbak opened this issue May 3, 2024 · 1 comment · Fixed by #6615
Assignees
Labels
effort: [S] 1-2 days of estimated development time priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

piotrbak commented May 3, 2024

Is your feature request related to a problem? Please describe.
In 3.16 we introduced the feature that's fetching the links from the homepage. Currently, this process is happening synchronously, whenever the critical images data is cleared.

It's causing potential problems on the slower environments, as we need to wait until the homepage is loaded and fetched synchronously. It can even cause timeouts.

On our testing environment, clearing the data took 13s compared to 2.4s when putting static data into fetch_links() function.

The further findings are:

  1. We're sending the blocking requests to saas also when debug mode is disabled
  2. The usleep between each request is set to 0.5s
  3. We're doing two requests to collect the links (one using mobile UA to collect mobile device links and the second one using desktop UA to collect desktop links)

Describe the solution you'd like

  1. Visit the homepage only once using mobile and populate all (desktop/mobile) links based on that
  2. Reduce the usleep when debug mode is disabled (the exact value needs discussion here and dev direction)
  3. Make the requests not blocking when the debug mode is disabled

Acceptance Criteria

  1. When fetching the links for desktop and mobile, we should do only one, mobile request. The links will be shared between mobile and desktop
  2. When debug mode is disabled, requests sent to the saas should not be blocking
  3. When debug mode is enabled, requests sent to the saas should be blocking te receive the log data
  4. When debug mode is disabled, we should reduce the usleep to minimum
  5. When debug mode is enabled, we should reduce the usleep to be lower than it's currently
  6. It would be great to measure time before/after to confirm the impact
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: high Issues which should be resolved as quickly as possible lcp labels May 3, 2024
@piotrbak piotrbak added this to the 3.16 milestone May 3, 2024
@jeawhanlee
Copy link
Contributor

Scope a solution ✅

Engine\Media\AboveTheFold\WarmUp::Controller

  • In warm_up

    • Remove the conditional check for mobile and also the conditional block
    • Keep the user_agent for mobile and remove the one for desktop
  • In fetch_links

    • Remove the device param
    • Keep the user_agent for mobile and remove the one for desktop
  • In send_to_saas

    • Remove the device param in the add_to_atf_queue method
    • Add a conditional check for mobile and add the mobile version to queue.
    foreach ( $links as $link ) {
                    $this->api_client->add_to_atf_queue( $link );
                        if ( $this->is_mobile() ) {
                            $this->api_client->add_to_atf_queue( $link, 'mobile' );
                        }
                    usleep( $delay_between );
               }
    
  • Add a conditional check for WP_ROCKET_DEBUG if defined and true to reduce the usleep.

  • For non-blocking request sent to SaaS, we currently share the same method for RUCSS and LCP which is located here if we want to continue with the non-blocking request, then we'll need to update this class to extend WP_Rocket\Engine\Common\JobManager\APIHandler::AbstractAPIClient and create a new add_to_queue method that sends a non-blocking request.

  • Update tests.

Estimate the effort ✅

[S]

@jeawhanlee jeawhanlee added the effort: [S] 1-2 days of estimated development time label May 3, 2024
@remyperona remyperona self-assigned this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants