Closed (outdated)
Project:
Drupal core
Version:
9.5.x-dev
Component:
menu system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 Sep 2010 at 15:41 UTC
Updated:
25 Jun 2023 at 22:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
sunStill the same overall logic, but heavily improved + clarified. No functional changes.
Comment #2
moshe weitzman commentedThis pattern is going to drive me to drink.
We have a single table query here with all the right indices. There is normally no reason to cache this. But folks abuse the caching layer as a way to avoid using the DB at all. Cachein, and especially persistent cache, is not free. It introduces up thorny invalidation/expiration issues.
I'm not going to actively block this, but I won't promote it either.
Comment #3
moshe weitzman commentedComment #4
sun@moshe: I've to agree with you, and am equally baffled by the introduction of so many separate and manually built caches throughout D7, leading to lots of duplicate code, no clear pattern, and the potential of invalidation/expiration problems. We should have done #636454: Cache tag support prior to introducing all those caches, but it seems several core contributors wanted to make core perform faster for sites leveraging a fast in-memory cache (so as to not hit the database even once for simple, regular requests), regardless of that cache invalidation API. For that sake, I'm just continuing that idea here.
In this case, there should not be any invalidation/expiration issue, as the data is cached in {cache_menu}, which is used for identical cache data by the menu system, and already cleared at the right times.
Comment #5
sunAlthough my local box is totally not suitable for doing benchmarks, I've mass-applied the following range of current performance patches and am getting a nice boost:
#910190: Entire schema cache needlessly loaded on all pages
#910156: Remove 'description' keys from schema cache
#606966: 'load arguments' of parent path are not inherited
#910124: Clean up menu_contextual_links()
Test page: admin/config/development/devel; Garland; enabled Navigation menu block containing 1 taxonomy term link and 1 node link.
Comment #6
moshe weitzman commentedthats fine, but you didn't get that from removing a single fully indexed query which is whats proposed here.
fyi, i don't think that invalidation API is really the answer for this fixing this anti-pattern. people really need to trust their database (and refactor queries which make their DB unstable).
Comment #7
catchI don't think the cache invalidation API solves the 'caching single indexed queries' either, what I will say is that on benchmarks on ex2 only two types of queries show up in the MySQL slow query log:
1. cache misses for queries that are cached (usually things that can only be cached per page, and the load tests only return to something like 5-10% of pages per run if that)
2. a few queries like node_get_types() that aren't cached.
Additionally, while you can scale MySQL for reads via replication, you can only do that for slave-safe queries - that's not the case for queries on any system-level information like the menu system, system table, field configuration which always need to be up-to-date. memcache, while it needs to be cleared at the right time, doesn't suffer from replication lag. So while these patches don't result in a significant performance improvement for an individual request, they do result in being able to scale to a much higher number of requests per second before hitting the limits of MySQL.
Having said that, generally the only people able to view contextual links are going to be authenticated users with some kind of additional permissions, so I'm not sure that argument holds in this case. Generally I've restricted query culling to things which run for anonymous users with no page caching.
Comment #8
moshe weitzman commentedcatch's argument about not messing with admin/authenticated features like contextual links is pretty compelling.
that makes sense that this anti-pattern is useful for scaling and not page speed. but i still think you have to attack the root cause which are queries that are gumming up mysql. if you don't attack root cause, you end up where we are now which is afraid to depend on your database. you start caching the hell out out of your app which is hard for invalidation, testing, debugging.
Comment #9
sunThanks for the clarification.
But, ugh, I took the chance to quite heavily clean up that menu_contextual_links() function, which I'd still like to get in.
Comment #10
sunAlthough badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).
Comment #11
kscheirer#9: drupal.menu-contextual-links.9.patch queued for re-testing.
Comment #25
smustgrave commentedWonder if this is still needed for D10?
Comment #26
avpadernomenu_contextual_links()does not exist in Drupal 9.5. Either the issue summary needs to be updated, or this issue can be closed because there is not a function/method that contains the code that was inmenu_contextual_links().Comment #27
avpadernoI guess that code should be now in the
ContextualLinkManagerclass which, as other plugin managers, uses a cache already.Comment #28
smustgrave commentedAgreed. Think this can be closed then. If anyone disagrees please reopen updating issue summary for D10
Thanks!