It seems as though menu.inc needs to clear the page and block cache when a menu item is saved or deleted, or the menu router is rebuilt? Since the menus may be displayed in cached blocks and/or on cached pages this seems pretty important.

of course, this means that sometimes it would get cleared twice (e.g on node_save for a node with a menu_orf menu item or in a book).

CommentFileSizeAuthor
#17 menu_less_cache_clears-170514-17.patch3.56 KBpwolanin
#15 menu_less_cache_clears-170514-15.patch3.48 KBpwolanin
#12 menu_less_cache_clears-170514-12.patch2.27 KBpwolanin
#11 menu_less_cache_clears-170514-11.patch2.29 KBpwolanin
#7 menu_less_cache_clears-170514-7.patch2.07 KBpwolanin
#5 menu_less_cache_clears-170514-5.patch744 byteschx
cache_clear_menuinc_1.patch1.84 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

patch still applies with offset

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

this is pretty trivial - any reason not to proceed with this?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Yes -- though the menu blocks themselves are not cacheable it's still a lot safer to blow the cache.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

chx’s picture

chx CreditAttribution: chx commented
Title: menu.inc should clear page and block caches » menu.inc issues too many cache_clear_alls
Status: Fixed » Needs review
FileSize
menu_less_cache_clears-170514-5.patch744 bytes

http://drupal.org/node/181758 warns us that menu_link_save will issue one cache_clear_all for each link which is very unnecessary.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

huh? That doesn't patch doesn't seem to make sense. No matter where or why menu_link_save is called the cache should be cleared once

Maybe a static var in menu_link_save()?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
menu_less_cache_clears-170514-7.patch2.07 KB

how about something more like this - also clears out a now unused param for the link deletion. We may also be able to remove the cache_caler_all() in function menu_rebuild() but I'm not sure.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

I love this approach.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Needs work

Erm, if we clear the cache on the end of the page request in a shutdown function, the cache will not be rebuilt through the page request handling. For example the deleted menu item will still be in the page generated, and will only disappear on the next page reload. Let's discuss this.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Gabor - I don't think that's likely happen.

If the page load is saving or deleting a link, then you have almost certainly just submitted a form. The saving/deleting happens during the page load that handles the form submission, then you'll usually get a redirect. I guess you might get stale data shown if you have a form that does not redirect after submission.

The alternatives are to either clear the cache many times (potentially) or to only clear it after the first link is saved or deleted. I was trying to figure out whether this latter approach could also lead to problems - for example another user start a page load after the cache is cleared but before additional links are saved or deleted.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
menu_less_cache_clears-170514-11.patch2.29 KB

Here's a patch that should avoid all possible race conditions by clearing the cache up to two times. Is this too complicated?

I just whipped this up, so it needs some testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
menu_less_cache_clears-170514-12.patch2.27 KB

fixes indenting

DrupalTestbedBot tested pwolanin's patch (http://drupal.org/files/issues/menu_less_cache_clears-170514-11.patch), the patch passed. For more information visit http://testing.drupal.org/node/114

DrupalTestbedBot tested pwolanin's patch (http://drupal.org/files/issues/menu_less_cache_clears-170514-12.patch), the patch passed. For more information visit http://testing.drupal.org/node/115

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
menu_less_cache_clears-170514-15.patch3.48 KB

Moshe pointed out in an e-mail that there are still an absurd number of cache clears upon menu rebuild - the come from other calls in menu_link_save() the specifically clears part of the cache_menu table, and also from calls to variable set.

Attached patch deals now with all 3 cases and greatly reduces the number of calls (also suggests that devel module needs a little reworking to make sure its shutdown functions are last)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

note - when using devel module's "empty cache" function, this patch reduces for me the number of queries from 1626 to 901.

related patch for devel module too: http://drupal.org/node/182836

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
menu_less_cache_clears-170514-17.patch3.56 KB

slight tweaking - we may want to check expanded menus twice also to minimize the possibility of having incorrect menus displayed.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

patch still applies with offset

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

The easiest way to evaluate this patch is to have the Devel module installed - look at the number of queries - for example on admin/build/modules. I see this patch reduce the number of queries on that page from 1773 to 1009.

The logic is also designed to protect against race conditions on busy sites, but it's a little hard to test that directly.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

note - the patch also removes a bit of code cruft - the now unused and unneeded 2nd parameter to function _menu_delete_item()

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

I have checked the patch and it's ready to go.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.