search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[PS] Optimize menu_navigation_links_rebuild() #5389

Open hosef opened 2 years ago

hosef commented 2 years ago

Description of the need

The function menu_navigation_links_rebuild() uses a large number of database queries when rebuilding the system menu links that are created based on hook_menu. This process uses nearly 300 database queries without any contributed modules, and gets more expensive the more modules you have.

Proposed solution

These queries can be statically cached during the process and then all written back to the database at the end. Based on my initial testing, this reduces test execution time by 10-15%

Additional information

I had to modify one test that deletes menu items without using menu_link_delete to simulate a database crash. The behavior in this case is changed because the links are stored using a static cache.

PR: https://github.com/backdrop/backdrop/pull/3659

Advocate: @hosef

ghost commented 2 years ago

I fear that reviewing this PR is above my level of expertise, but I like the sound of these changes!

I also think that any issue with the word 'optimise' in the title and a matching PR made by @hosef should be put on the fast-track to be merged 😉

docwilmot commented 2 years ago

I also think that any issue with the word 'optimise' in the title and a matching PR made by @hosef should be put on the fast-track to be merged 😉

Seconded. I was going to add it to milestones to get on @quicksketch radar but I forgot we need a candidate tag then a second. I've tagged, can you milestone @BWPanda?

ghost commented 2 years ago

Unfortunately not @docwilmot. We'd either need an advocate, or the PR needs to be RTBC before we can add the minor milestone.

hosef commented 2 years ago

I would be fine being the advocate for this issue.

docwilmot commented 2 years ago

I've taken a while to go over this, and I cannot find issue with it. I am however not intimately familiar with the menu system, so this will require more scrutiny from a committer, but that happens anyway. So marking RTBC.

One comment would be if there is an opportunity to avoid one more db query here but that would not affect the fact that the PR as is seems good to me.

hosef commented 2 years ago

@docwilmot That link takes me to the top of the PR code changes, but doesn't highlight a specific line. Which section were you referencing?

docwilmot commented 2 years ago

Oops, the comment for that line "// Updated and customized items whose router paths are gone need new ones"

hosef commented 2 years ago

@docwilmot I reworked that section. could you take a look at it again.

jenlampton commented 2 years ago

Removing needs code review label to get this onto the desks of core committers :)

quicksketch commented 2 years ago

I skimmed over the code to check the approach and I think it's a great idea. We do something very similar when config changes, we update the static cache instead of re-reading the files from disk.

I'll give this a full read through soon if another committer doesn't get to it first.

quicksketch commented 2 years ago

I benchmarked this using XHProf and found the results to be pretty convincing on a full cache clear (using the Flush all caches button on admin/config/development/performance).

image

To test I did the following:

I sorted by "Inclusive Wall Time Diff" for this comparison, since I wanted to see how different menu_rebuild() was in this situation. From the results above, you can see that the overall cache rebuild time was improved 17%, while the menu rebuild itself was improved 107%, a 2x improvement in menu_rebuild() time.

You can also see a reduction of 367 SELECT queries calls in the screenshot above.

However, at the same time, memory usage also increased 19% and menu-specific memory usage increased 100% (approximately a 1MB increase in my case). This was with 12 primary menu links and the standard out-of-box admin menu of 140 links.

In my situation, that meant that menu_rebuild() simultaneously became twice as fast while consuming twice as much memory. However, a big differentiator here is that this is on a 2021 machine with a very fast processor and SSD. On most web hosting, the improvement in speed might be more dramatic, but the memory usage would always be the same. Meaning on slower databases or disks, you'd get a bigger performance-to-memory trade-off.

Performance-wise, this gets a big thumbs-up from me. :+1:

quicksketch commented 2 years ago

I re-read over the code and the only thing of note is that I think some docblock could be improved: https://github.com/backdrop/backdrop/pull/3659#pullrequestreview-842187344

I was trying to think if there was a way to reduce the memory usage after a menu_rebuild(), such as using unset() on the cache after the rebuild was finished, but because the cache is used in _menu_link_find_parent(), menu_link_children_relative_depth(), and menu_link_save() as well, that same cache might be necessary dozens of times in a single request. As such we probably need to leave it in place for the entirety of the request. Fortunately it's only used in rebuild/update situations, and doesn't affect memory usage once the menu has been built.

@hosef if you could review and make any last changes to the docblock I mentioned that would be great! Leaving this RTBC because it's not really a blocker for merging.

klonos commented 2 years ago

I was trying to think if there was a way to reduce the memory usage after a menu_rebuild(), such as using unset() on the cache after the rebuild was finished, but because the cache is used in _menu_link_find_parent(), menu_link_children_relative_depth(), and menu_link_save() as well, that same cache might be necessary dozens of times in a single request. As such we probably need to leave it in place for the entirety of the request. Fortunately it's only used in rebuild/update situations, and doesn't affect memory usage once the menu has been built.

Shouldn't this be reworded and documented in a docblock as well? (so that others that wonder the same may know why we're not doing it)

quicksketch commented 2 years ago

I made an extensive docblock update to my suggestion in the PR: https://github.com/backdrop/backdrop/pull/3659/files#r776950727

I was originally a little sparse on the documentation because it's an internal function that shouldn't be needed outside of core menu functions, but you're absolutely right that having good docs will help with core maintenance in the future.

klonos commented 2 years ago

Thanks @quicksketch ...looks good to me 👍🏼

quicksketch commented 2 years ago

I merged https://github.com/backdrop/backdrop/pull/3659 into 1.x for 1.21.0 (after apply the docblock updates approved by @klonos).

Great work @hosef on this enhancement! Thank you @jenlampton, @docwilmot, and @klonos for reviewing these changes!

klonos commented 2 years ago

This change was reverted because of the regression it caused in the 1.21.0 release. See: #5460

Reopening (but feel free to close again if we've given up on the idea).