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

βœ… πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks - [ED-10756] #21762

Closed
5 tasks done
MajorChump opened this issue Mar 25, 2023 · 22 comments
Labels
bug Indicates a bug with one or multiple components. mod* solved Indicates that an Issue has been Solved, or a Feature Request has been Released. status/merged Indicates when a Pull Request has been merged to a Release. type/performance Indicates when a topic is related to Performance.
Milestone

Comments

@MajorChump
Copy link

MajorChump commented Mar 25, 2023

Prerequisites

  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • The issue still exists against the latest stable version of Elementor.

Description

I've started profiling with XHPROF to determine why a new Elementor navigation bar reduced our page load times by around 5.5 seconds. The nav bar is complex and has a lot of elements.

From profiling I've found some very noticeable performance issues with a number of functions. Most of the time is wasted inside Controls_Stack. From my light understanding this is used for each Element on a page to create an array of settings. The building of these arrays is where the performance issues with Elementor lie.

I've managed to make a number of small changes to this existing code which has around halfed the load time and will share a PR for these soon. I'm raising this issue as a complete rethinking of this class and methods is needed and my understanding of Elementor's codebase is extremely limited.

For each Element added to a page we end up calling Controls_Stack::get_active_settings around 10 times and I've not been able to understand why. This call is extremely expensive as it calls a number of other methods which all use get_controls and loop through every control. Individually this isn't expensive but these loops are adding up.

For a page which I believe has around 60 Elementor sections.

Screenshot 2023-03-25 at 09 39 33

Screenshot 2023-03-25 at 09 32 33

callgraph (1)

We're ending up with some methods being called 300k+ times.

It seems to me this code has gone astray and duplicate work is being done over and over again. Here is the same page run with some of my improvements but half of eternity is still an eternity for something that should be simple.

Screenshot 2023-03-25 at 09 41 20

Steps to reproduce

N/A

Isolating the problem

  • This bug happens with only Elementor plugin active (and Elementor Pro).
  • This bug happens with a Blank WordPress theme active (Hello theme).
  • I can reproduce this bug consistently following the steps above.

System Info

N/A

@MajorChump MajorChump added the status/awaiting_triage Indicates when an Issue, Pull Request, or Discussion awaits to be triaged. label Mar 25, 2023
@MajorChump
Copy link
Author

MajorChump commented Mar 25, 2023

#21763 50%+ reduction as detailed above

@m1ga
Copy link

m1ga commented Mar 26, 2023

awesome investigation! Hopefully someone from the Elementor team will pick it up and verify it. 50% difference is a lot πŸ˜„

@shilo-ey
Copy link
Contributor

Thank you @MajorChump !

We actually recently worked on optimizing this part of code, which is not optimal, as you mentioned.

When trying to use the version generated by the PR you just sent we didn't manage to make the editor nor the frontend to load, but you indeed provided with a different point of view over the issue that we will further investigate.

Keeping this thread open for reference for now. if you manage to make it work on your site please make sure you are using Elementor features (additional breakpoints, optimized DOM, asset loading etc to make sure it works)

Feel free to contribute to this discussion,

Thanks!

@shilo-ey shilo-ey added status/has-pr Indicates that an Issue, or Discussion has a companion Pull Request awaiting to be merged. type/performance Indicates when a topic is related to Performance. labels Mar 28, 2023
@MajorChump
Copy link
Author

MajorChump commented Mar 28, 2023

@shilo-ey Thanks we are running this live on our site, I did have to copy the changes across to the git repository so its possible I made a mistake then, we have all of those features active.

Did you get any errors?

The two biggest gains with this PR are:

  1. Eliminating the duplication of work by getting a Base_Data_Control for each type. This was happening thousands of times yet I only saw 41 unique types of Base_Data_Control.
  2. Passing the controls array to is_control_visible as this method is being called 320k and each call it calls get_controls which has a decent amount of overhead. I have no idea the impact of this, but it seemed to work on our site and nothing broke.

To be fair I don't think this PR is the best way to proceed was just showing conceptually what we're facing. The class needs reducing to a single loop, I have a limited understanding of what each method is trying to do, but the bottlenecks lie with the loops within loops.

Happy to discuss further on slack or discord if it helps?

@nicholaszein nicholaszein changed the title TTFB Performance - XHPROF Profiling identifying code bottlenecks. 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks. Apr 1, 2023
@nicholaszein nicholaszein removed the status/awaiting_triage Indicates when an Issue, Pull Request, or Discussion awaits to be triaged. label Apr 1, 2023
@collinversluis
Copy link

+1 a php profile of the code shows is_control_visible and get_active_settings taking enormous amounts of time and processing power. The loops are at nauseum for is_control_visible. Users are being blinded by caching, redis, and varnish to think their elementor sites are performant. When on localhost or have intense traffic you immediately uncover the issues.

@at-benni
Copy link

at-benni commented Aug 3, 2023

I've got the same problem, any updates on the progress?

@at-benni
Copy link

at-benni commented Aug 3, 2023

grafik

@m1ga
Copy link

m1ga commented Aug 3, 2023

I'm not sure if this is helping but there was a performance patch merged into master
https://github.com/elementor/elementor/pull/23273/files
it looks like it gets the controls once and not everytime.

You could test the master branch to see if that makes any difference

@at-benni
Copy link

at-benni commented Aug 3, 2023

It did improve a bit but Elementor is still kinda slow on my site
grafik

@m1ga
Copy link

m1ga commented Aug 3, 2023

oh, nice that you could test it! The PR assigned to this ticket still looks different (and is open) and there is another controls perf poc (#23013). So maybe they will improve some parts in the next releases.

@at-benni
Copy link

at-benni commented Aug 3, 2023

Some more improvements with #23013 , love it :D
grafik

@MajorChump
Copy link
Author

MajorChump commented Aug 3, 2023

#23273 looks to do part of my PR, which was a reasonable gain 1. Eliminating the duplication of work by getting a Base_Data_Control for each type. This was happening thousands of times yet I only saw 41 unique types of Base_Data_Control.

#23013 looks interesting thats fixing the merging of the setting arrays which is were the main issue was when I opened this PR. This PR was never meant for merging though it was just to show how I achieved a 50%+ reduction. I'll keep an eye on #23013 as this could offer a dramatic improvement.

@m1ga
Copy link

m1ga commented Aug 3, 2023

@MajorChump it's awesome that you've made the test in the first place otherwise they wouldn't have started to dig around

@MajorChump
Copy link
Author

Just tested both PR's on top of master and seeing 33% reduction on page load time. I'll do some further profiling when I have a minute.

@at-benni
Copy link

at-benni commented Aug 4, 2023

I've had a 50% reduction on page load time with the main branch https://github.com/elementor/elementor/pull/23273/files + #23013 applied
`root@BL:/mnt/c/Users/bl# httpst 5 https://mydomain
Measuring 5 requests to: https://mydomain
[##################################################] 100%

Median Response Time: 3.138649 seconds
Fastest Response Time: 3.060448 seconds
Slowest Response Time: 3.300941 seconds
0 requests failed
Final url was https://mydomain/
root@BL:/mnt/c/Users/bl# httpst 5 https://mydomain
Measuring 5 requests to: https://mydomain
[##################################################] 100%

Median Response Time: 6.284483 seconds
Fastest Response Time: 5.689416 seconds
Slowest Response Time: 6.784642 seconds
0 requests failed
Final url was https://mydomain/`
(this is not a production site)

@Nevoss
Copy link
Contributor

Nevoss commented Aug 6, 2023

Hey @MajorChump Thanks a lot for your help in those PRs
#23273 #23273

@Nevoss
Copy link
Contributor

Nevoss commented Aug 6, 2023

#23273 looks to do part of my PR, which was a reasonable gain 1. Eliminating the duplication of work by getting a Base_Data_Control for each type. This was happening thousands of times yet I only saw 41 unique types of Base_Data_Control.

#23013 looks interesting thats fixing the merging of the setting arrays which is were the main issue was when I opened this PR. This PR was never meant for merging though it was just to show how I achieved a 50%+ reduction. I'll keep an eye on #23013 as this could offer a dramatic improvement.

Did you see some improvement in this PR? because I have tested it out, and it seems that the improvement is really really small, maybe you can send the JSON of the page in which you can see a meaningful improvement

@nicholaszein nicholaszein changed the title 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks. πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks. Aug 20, 2023
@nicholaszein nicholaszein added this to the 3.16.0 milestone Sep 8, 2023
@kangarko
Copy link

grafik

@benni1516 Please which plugin are you using to render these data?

@neylwalecki
Copy link

@MajorChump you rock! πŸ‘ πŸš€ thanks

@rwkyyy
Copy link

rwkyyy commented Sep 13, 2023

@kangarko looks like query monitor, but I might be mistaken.

@collinversluis
Copy link

grafik

@benni1516 Please which plugin are you using to render these data?

@rwkyyy @kangarko It's https://code-profiler.com/

@nicholaszein nicholaszein added status/merged Indicates when a Pull Request has been merged to a Release. and removed status/has-pr Indicates that an Issue, or Discussion has a companion Pull Request awaiting to be merged. labels Sep 23, 2023
@nicholaszein nicholaszein changed the title πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks. πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks - [ED-10756] Sep 23, 2023
@nicholaszein nicholaszein changed the title πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks - [ED-10756] βœ… πŸ“Š 🐞 Bug Report: TTFB Performance - XHPROF Profiling identifying code bottlenecks - [ED-10756] Sep 23, 2023
@nicholaszein
Copy link
Contributor

Hello everyone!

We have great news! πŸ™Œ

πŸ“’ We're happy to announce that the issue you raised was resolved in Elementor Core v3.16! πŸ₯³

βœ… Feel free to check it out and update your plugin to the new version!

Check the changelog for more information:
Changelog of Elementor and Elementor Pro

Cheers πŸ₯‚

@elementor elementor locked and limited conversation to collaborators Sep 23, 2023
@nicholaszein nicholaszein added the solved Indicates that an Issue has been Solved, or a Feature Request has been Released. label Sep 24, 2023
@nicholaszein nicholaszein added the bug Indicates a bug with one or multiple components. label Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Indicates a bug with one or multiple components. mod* solved Indicates that an Issue has been Solved, or a Feature Request has been Released. status/merged Indicates when a Pull Request has been merged to a Release. type/performance Indicates when a topic is related to Performance.
Projects
None yet
Development

No branches or pull requests

10 participants