-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation: Fix performance regression #58513
Conversation
I did a bit of an analysis. We have a "function bag" class WP_Navigation_Block_Renderer, and its entry point is WP_Navigation_Block_Renderer::render. Here is the flow that appears in the code:
We can see above that Either we fix |
Co-authored-by: Andrei Draganescu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for looking into this, it fixes the issue: the navigation block is back to rendering the inner blocks once.
Trunk | This PR | |
---|---|---|
Total (ms) | 631 | 532 |
gutenberg_render_block_core_navigation (ms) |
103 | 16 |
# calls to WP_Block->render |
439 | 434 |
# calls to gutenberg_render_block_core_page_list |
6 | 1 |
I don't want to share % of performance, because this analysis is using a profiler (xdebug). This is good for understanding whether it fixes the issue, but not for gauging the impact in production. I'll share this later.
I'm sharing my before/after cachegrind files: cachegrind-files.zip. Anyone can open them with a tool such as
This is the step-by-step process I used to get them, so anyone can replicate.
# Create a `.wp-env.override.json` with the following contents.
{
"config": {
"WP_DEBUG": false,
"SCRIPT_DEBUG": false
}
}
# List the files. They should be something like cachegrind.out.PID.gz.
docker exec -it `docker ps -f name=e-wordpress -q` ls -lat --time-style=+%H:%M:%S /tmp/
# Take the latest one, which corresponds with the front-page load.
docker cp `docker ps -f name=e-wordpress -q`:'/tmp/cachegrind.out.PID.gz' ~/Desktop/
gzip -d ~/Desktop/cachegrind.out.PID.gz
|
There are some e2e errors related to navigation. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
What?
This is another attempt at #58506.
In #55605 we changed the navigation rendering logic so that inner_blocks were rendered multiple times. This updates that logic so that inner_blocks are only rendered once.
Why?
To improve performance.
How?
Instead of calling
render
every time we want to check that a navigation has a submenu, we now save that as a member variable on the class. This means we only need to call render once for each inner block.There should be a more performant way to determine if a navigation contains submenus.
Testing Instructions
Check that a navigation block with submenus displays as on trunk.