Skip to content

Fixes #4072 Prevent high CPU load from resource fetcher#4139

Merged
remyperona merged 8 commits intodevelopfrom
fix/4072-resource_fetcher_high_CPU_load
Jul 16, 2021
Merged

Fixes #4072 Prevent high CPU load from resource fetcher#4139
remyperona merged 8 commits intodevelopfrom
fix/4072-resource_fetcher_high_CPU_load

Conversation

@engahmeds3ed
Copy link
Copy Markdown
Contributor

@engahmeds3ed engahmeds3ed commented Jul 7, 2021 •
edited by remyperona
Loading

Description

We splitted the processing time between ResourceFetcher and ResourceFetcherProcess as follows:-

ResourceFetcher Responsibility

  1. Collect CSS/JS resources from the HTML.
  2. Get URL paths for each resource.
  3. Send URL and path to the ResourceFetcherProcess background process.

ResourceFetcherProcess Responsibility

  1. Receive CSS/JS resources (URL, path).
  2. Get the content of the file based on its type (internal or external)
  3. Minify the content.
  4. Check the file on the local database.
  5. Call the SaaS warmup.

So we moved the content-related methods to be on ResourceFetcherProcess

Fixes #4072

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Is the solution different from the one proposed during the grooming?

Yes exactly the same

How Has This Been Tested?

  • Tested pre-warmup locally
  • Tested normal warmup locally
  • Adjust tests to pass.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@engahmeds3ed engahmeds3ed self-assigned this Jul 7, 2021
@engahmeds3ed engahmeds3ed added module: remove unused css priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior labels Jul 7, 2021
@engahmeds3ed engahmeds3ed requested a review from a team July 7, 2021 13:08
@remyperona remyperona marked this pull request as ready for review July 7, 2021 13:13
@remyperona remyperona added this to the 3.9.1 milestone Jul 7, 2021
@engahmeds3ed
Copy link
Copy Markdown
Contributor Author

engahmeds3ed commented Jul 7, 2021

@wp-media/qa

To test this change we need to make sure of the following:-

  • With fresh installation, try pre-warmup by enabling the RUCSS option for the first time then check the DB resources table and make sure that resources are added to this table with proper content.
  • Fetching process should be faster than before.
  • If WordOps (the infra that you use at our QA sites) has a CPU usage meter you can use it to test that after this change the CPU usage should be lower and also bandwidth.
  • We should also test all types of resources' URLs (relative, absolute)

@engahmeds3ed engahmeds3ed requested a review from a team July 7, 2021 13:35
@Mai-Saad Mai-Saad self-requested a review July 7, 2021 14:31
@remyperona remyperona changed the title Fix resource fetcher high cpu load Fixes #4072 Prevent high CPU load from resource fetcher Jul 7, 2021
@engahmeds3ed engahmeds3ed requested a review from remyperona July 12, 2021 20:28
Mai-Saad
Mai-Saad reviewed Jul 15, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Mai-Saad Mai-Saad left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing is done.
With the latest recheck of performance after external files change => Total Fetch time increased (compared to trunk) while CPU utilization decreased

Notes:

@remyperona
Copy link
Copy Markdown
Contributor

remyperona commented Jul 15, 2021

Fetch time increase can make sense since we're now doing more processing later instead of upfront, so each individual fetch job will take longer. The difference is small, which is acceptable.

The reduction in CPU load is more important in that case.

@remyperona remyperona merged commit 68ae93b into develop Jul 16, 2021
@remyperona remyperona deleted the fix/4072-resource_fetcher_high_CPU_load branch July 16, 2021 13:01
remyperona pushed a commit that referenced this pull request Jul 21, 2021
@engahmeds3ed @remyperona
Fixes #4072 Prevent high CPU load from resource fetcher (PR #4139)
3a38e22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remyperona remyperona remyperona approved these changes

@Mai-Saad Mai-Saad Mai-Saad approved these changes

Assignees

@engahmeds3ed engahmeds3ed

Labels

module: remove unused css priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior

Projects

None yet

Milestone

3.9.1

Development

Successfully merging this pull request may close these issues.

High CPU usage related to rocket_saas_warmup async process

3 participants

@engahmeds3ed @remyperona @Mai-Saad