Menus: Fix performance issue with tons of duplicated queries #8140

arzola · 2025-01-17T04:08:19Z

The start_el() method in Walker_Nav_Menu currently calls get_privacy_policy_url() for every menu item when building menus. This results in redundant queries, particularly for menus with many items. This patch initialize the get_privacy_policy_url() result in the Walker constructor, improving performance while maintaining existing functionality.

Trac ticket: https://core.trac.wordpress.org/ticket/62818

How to Test

1.	Install and activate the [Query Monitor](https://en-ca.wordpress.org/plugins/query-monitor/) plugin.
2.	Create a menu with multiple items (20+ for better observation).
3.	Assign the menu to a theme location that uses wp_nav_menu() to render it.
4.	Enable the privacy policy page under Settings > Privacy to ensure get_privacy_policy_url() returns a value.
5.	Load a page where the menu is rendered.

Before applying the patch:

* Use Query Monitor to observe database queries.
* Notice multiple redundant calls to get_privacy_policy_url() for each menu item.

After applying the patch:

* Reload the same page and monitor queries using Query Monitor.
* Confirm that get_privacy_policy_url() is only called once, even with many menu items.

Commit Message

Menus: Improve performance by calling get_privacy_policy_url() once per Walker_Nav_Menu instance rather than for every nav menu item.

The start_el() method in Walker_Nav_Menu was calling get_privacy_policy_url() for every menu item when building menus. This resulted in redundant queries, particularly for menus with many items. This patch obtains the get_privacy_policy_url() value in the constructor for reuse in the start_el() method to improve performance.

Redundant code to construct the privacy policy page is also refactored into the set_up method during tests.

Props arzola, swissspidy, westonruter, mukesh27.
Fixes #62818.

The start_el() method in Walker_Nav_Menu currently calls get_privacy_policy_url() for every menu item when building menus. This results in redundant queries, particularly for menus with many items. This patch memoizes the get_privacy_policy_url() result, improving performance while maintaining existing functionality.

github-actions · 2025-01-17T04:08:43Z

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props arzola, swissspidy, westonruter, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2025-01-17T04:08:45Z

Hi @arzola! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

github-actions · 2025-01-17T04:30:18Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

swissspidy · 2025-01-17T09:25:36Z

src/wp-includes/class-walker-nav-menu.php

+	 *
+	 * @return string The URL to the privacy policy page.
+	 */
+	public static function check_privacy_policy_url() {


There is no reason for this to be public. This can be a private method.

Also, I would rename to get_privacy_policy_url()

yes I agree, also I fixed the tests to behave as expected

swissspidy · 2025-01-17T09:27:22Z

src/wp-includes/class-walker-nav-menu.php

+	 * This method caches the URL to avoid multiple database queries and shared the URL between all instances of the class.
+	 * Single query is performed on the first call to this method.
+	 *
+	 * @since 6.7.2


This is not something for a point release, as this is not a bug introduced in 6.7. (It was added in 6.2)

Let's change this to 6.8.0

arzola · 2025-01-17T13:10:52Z

tests/phpunit/tests/menu/walker-nav-menu.php

+	public static function set_up_before_class() {
+		parent::set_up_before_class();
+
+		$post_id = self::factory()->post->create(


I centralized the privacy policy page generation to run only once for the entire test class, it was the same code in each test and there was no side effects

arzola · 2025-01-17T14:36:51Z

I just fixed the tests behaviour, they were running well in isolation but failing when running all

westonruter · 2025-01-17T17:53:58Z

src/wp-includes/class-walker-nav-menu.php

+	 * @since 6.2.0
+	 * @var string
+	 */
+	private static $privacy_policy_url = null;


Does this need to be static? It seems like it could be non-static, as this would solve potential problems with unit testing down the road, as well as the unlikely scenario where the privacy policy URL changes between different nav menu instances.

The thing is having this static allow us to share the same data across any instance of the class, if this "state" is not shared between instances we would query the database each time get_privacy_policy_url() is called in each instance of the Walker.

That's exactly the thing I want to prevent with this change because the privacy url is not a thing that changes regularly or that can be different across Walker instances, I feel is more performant and the right way to do it, probably we can just having one database call per walker (with a non static attribute), but if users uses several wp_nav_menu calls with the default walker this will sum up

I'm ok of having this non static if we consider that it's ok to query the database for each walker instance, maybe is not a big deal, I'm suffering with the test suite 🤡 not entirely sure why it fails when running the entire test suite considering I'm cleaning up the things I created

On that case I think this could be better and cleaner

/** * The URL to the privacy policy page. * * @since 6.2.0 * @var string */ private $privacy_policy_url = null; /** * Constructor. */ public function __construct() { $this->privacy_policy_url = get_privacy_policy_url(); }

How many walker instances are there on a typical page though? Two?

LOL I found the issue because a website I was trying to fix uses 6 wp_nav_menu some for mobile and some for desktop, but still I think this is needed because if a single menu has a lot of subitems only one walker instance will trigger N-menu-items queries. I'll remove the static I'm ok keeping the things simple and a little bit more performant.

I like your suggestion in #8140 (comment)

I updated the PR with the constructor approach, thanks for your feedback

src/wp-includes/class-walker-nav-menu.php

Co-authored-by: Weston Ruter <[email protected]>

westonruter

LGTM

arzola · 2025-01-17T23:04:32Z

Thank you @westonruter I'm kinda lost what's the next step hahaha, should I open a SVN PR?, not sure what to do

westonruter · 2025-01-17T23:08:33Z

Ah, the next step is for a committer to take your code and commit to SVN. So your work here is done!

mukeshpanchal27

Two minor feedbacks

src/wp-includes/class-walker-nav-menu.php

Co-authored-by: Mukesh Panchal <[email protected]>

spacedmonkey

LGTM

github-actions · 2025-01-21T21:25:36Z

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 59674
GitHub commit: 396f6fb

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

lint fix

6e755eb

swissspidy reviewed Jan 17, 2025

View reviewed changes

improve tests

1db8c0d

arzola commented Jan 17, 2025

View reviewed changes

arzola changed the title ~~Menus: Fix performance issue duplicated queries~~ Menus: Fix performance issue with tons of duplicated queries Jan 17, 2025

westonruter reviewed Jan 17, 2025

View reviewed changes

remove static call, use constructor instead

0042049

westonruter reviewed Jan 17, 2025

View reviewed changes

src/wp-includes/class-walker-nav-menu.php Outdated Show resolved Hide resolved

Update src/wp-includes/class-walker-nav-menu.php

72a29fb

Co-authored-by: Weston Ruter <[email protected]>

westonruter approved these changes Jan 17, 2025

View reviewed changes

mukeshpanchal27 reviewed Jan 21, 2025

View reviewed changes

src/wp-includes/class-walker-nav-menu.php Show resolved Hide resolved

src/wp-includes/class-walker-nav-menu.php Outdated Show resolved Hide resolved

arzola and others added 2 commits January 21, 2025 12:54

Update src/wp-includes/class-walker-nav-menu.php

b6baf83

Co-authored-by: Mukesh Panchal <[email protected]>

Update src/wp-includes/class-walker-nav-menu.php

94a2523

Co-authored-by: Mukesh Panchal <[email protected]>

spacedmonkey approved these changes Jan 21, 2025

View reviewed changes

github-actions bot closed this Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menus: Fix performance issue with tons of duplicated queries #8140

Menus: Fix performance issue with tons of duplicated queries #8140

arzola commented Jan 17, 2025 •

edited by westonruter

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

swissspidy Jan 17, 2025 •

edited

Loading

arzola Jan 17, 2025

swissspidy Jan 17, 2025

arzola Jan 17, 2025

arzola commented Jan 17, 2025

westonruter Jan 17, 2025

arzola Jan 17, 2025 •

edited

Loading

arzola Jan 17, 2025 •

edited

Loading

arzola Jan 17, 2025

westonruter Jan 17, 2025

arzola Jan 17, 2025

westonruter Jan 17, 2025

arzola Jan 17, 2025

westonruter left a comment

arzola commented Jan 17, 2025

westonruter commented Jan 17, 2025

mukeshpanchal27 left a comment

spacedmonkey left a comment

github-actions bot commented Jan 21, 2025

Menus: Fix performance issue with tons of duplicated queries #8140

Menus: Fix performance issue with tons of duplicated queries #8140

Conversation

arzola commented Jan 17, 2025 • edited by westonruter Loading

How to Test

Before applying the patch:

After applying the patch:

Commit Message

github-actions bot commented Jan 17, 2025 • edited Loading

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

Test using WordPress Playground

Some things to be aware of

swissspidy Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arzola commented Jan 17, 2025

Choose a reason for hiding this comment

arzola Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

arzola Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

arzola commented Jan 17, 2025

westonruter commented Jan 17, 2025

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

spacedmonkey left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2025

arzola commented Jan 17, 2025 •

edited by westonruter

Loading

github-actions bot commented Jan 17, 2025 •

edited

Loading

swissspidy Jan 17, 2025 •

edited

Loading

arzola Jan 17, 2025 •

edited

Loading

arzola Jan 17, 2025 •

edited

Loading