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 for Definition class to improve performance and reduce High CPU usage #6413

Closed
joejoe04 opened this issue Jan 26, 2024 · 5 comments · Fixed by #6977 · May be fixed by #6416
Closed

Improvements for Definition class to improve performance and reduce High CPU usage #6413

joejoe04 opened this issue Jan 26, 2024 · 5 comments · Fixed by #6977 · May be fixed by #6416
Assignees
Labels
dev initiative 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

@joejoe04
Copy link

We have received some suggestions about how the Definitions class can be altered to improve the plugin's performance and potentially decrease CPU usage. Specific details can be found here:
https://secure.helpscout.net/conversation/2490895571/471075/#thread-7472405567

Here is the full content of the suggested changes:

In the last weeks I have been running some performance tests and I noticed some points inside WP Rockets code.
After some light alterations i could archive a speedup of about 300 to 500 milliseconds.

Since this contact form is surely not the place to paste entire chunks of sourcecode, I'll try to outline what I've done with the least amount of code as possible.

In the "Definition" class add a simple method getTags() to "return array_keys($this->tags)".

In the "DefinitionAggregate" add two new properties "aliases" and "tags" and fill them inside "__construct()" and "add()" like this:

  • $this->aliases[$definition->getAlias()] = true;
  • foreach ($definition->getTags() as $tag) {$this->tags[$tag] = true;}
    this will allow removing the costly foreach from "has()" and "hasTag()" and just replace it with:
  • return isset($this->aliases[$id]); or
  • return isset($this->tags[$tag]); respectivly

It would also be possible to speed up "getDefinition()" as well by changing the $this->definitions to use the alias as index key and setting it as well inside "__construct()" and "add()" like this:

  • $this->definitions[$definition->getAlias()] = $definition;
    But since this will also remove any definitions with the same alias I'm not sure if this wouldn't break any of the plugins functionality.

Additional context
https://secure.helpscout.net/conversation/2490895571/471075/ - Ticket
https://wp-media.slack.com/archives/C43T1AYMQ/p1706280695093479 - Slack Discussion

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

@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: grooming dev initiative labels Jan 26, 2024
shdri added a commit to shdri/wp-rocket that referenced this issue Jan 30, 2024
@piotrbak piotrbak modified the milestone: 3.15.10 Feb 8, 2024
@piotrbak
Copy link
Contributor

Pinging you here too @MathieuLamiot

Do you think it's worth checking or doing R&D?

@MathieuLamiot
Copy link
Contributor

Yes it is, however this is not something we should change in our plugin but in a dependency. We would need to share this report with PHP League container maintainers.
I am adding this to the cooldown.

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Sep 10, 2024

@MathieuLamiot looking at it, it seems related to tags only on league container.

We add tags into the plugin, but I don't recall we are actually using them.
Maybe a quick way to get fixed on this is to create a custom DefintionAggregate (which we can easily pass to the container without changing the library original repo) without the tag logic and remove them from the plugin?

@MathieuLamiot
Copy link
Contributor

@CrochetFeve0251 You should directly discuss this topic with the plugin team and @Tabrisrp

@remyperona
Copy link
Contributor

We can remove tags usage from the plugin, since it's not compatible with the way we use ServiceProviders, as I discuss a couple of years ago with the dev of the container

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev initiative 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
6 participants