- Issue created by @berdir
- π¨πSwitzerland berdir Switzerland
When disabling RoutePreloader and all render cache bins, I get 77 calls in total to preloadRoutes() for an anonymous user on my frontpage only 6 of them actually fetch additional routes (including the two from redirect), many of them already deliberately preload their specific routes. and mostly happening within cacheable bits. LocalTask does all the node local tasks at once, a few menus, some weird bits like .
Worth noting: Our site building approach almost exclusively uses nodes with paragraphs, we basically have no other linked routes such as views or something.
In total, less than 100kb memory reported through that method by Blackfire.
With render caches (but page and dynamic page cache disabled), I'm getting exactly 2 as anon user, those from redirect. As an admin, on the user edit form with navigation module I get 800 calls to preloadRoutes(), 13 cache lookups. On the frontpage, it's 11 with render caches on, 17 with them off. 500-600kb memory usage for admin.
(I'm optimistic that a lot of these will actually go away if we can fully cache the navigation)
Is there a third option? Can we just remove the RoutePreloader, do we really need this?
- Merge request !11082Draft: Issue #3503843 Change reoute preloading to use bootstrap cache and only load used routes β (Open) created by berdir
- π¨πSwitzerland berdir Switzerland
Somehow missed π RoutePreloader loads a lot of routes Active . Will keep this separate for now to do some more testing as the approach is quite different.
- π¨πSwitzerland berdir Switzerland
Doing some tests with a different implementation.
Basically, I disabled the route preloader for now completely. On a production system that's been running for a bit, I see around 110 variations of route preloader caches with the current approach. I assume that without preloading a ton of routes on every request, there will be a lot more of those variations. Right now, the cache is always for a bunch of routes, keyed by a hash of those.
I'm changing that to using getMultiple() so we don't get many overlapping variations of different sets of routes. I'm not sure what the performance difference between a multi-cache load and a single one is. It's multiple rows, multiple cache items to expand and check. On its own, possibly slower, but I plan to combine it with the second idea:
As mentioned, I'm playing with the idea of using a ChainedFast backend for either some (either hardcoded on not /admin or by still using the Preloader, but only to feed the list of "frontend" routes) or all. For the mix, it would split the routes, and load them from the fast or regular bin. At this point, getMultiple() a bunch of routes should be a lot faster than the regular query.
Right now we cache the serialized strings because we know we don't need all of them. Without the big preload, that's no longer true and we will use those routes quite reliable. So we could consider to stop doing that and benefit from improvements such as π Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint Active by caching the route objects and letting the cache backend/serializer optimize it. would also
For size comparison as to how big the router table is, on our install profile:
select count(*), sum(length(route)) from router; +----------+--------------------+ | count(*) | sum(length(route)) | +----------+--------------------+ | 876 | 1244156 | +----------+--------------------+ select count(*), sum(length(route)) from router where path not like '/admin%'; +----------+--------------------+ | count(*) | sum(length(route)) | +----------+--------------------+ | 175 | 231266 | +----------+--------------------+ MariaDB [db]> select count(*), sum(length(data)) from config; +----------+-------------------+ | count(*) | sum(length(data)) | +----------+-------------------+ | 1078 | 2124343 | +----------+-------------------+ MariaDB [db]> select count(*), sum(length(data)) from cache_discovery; +----------+-------------------+ | count(*) | sum(length(data)) | +----------+-------------------+ | 40 | 1454662 | +----------+-------------------+
Full router table is around 1.2MB, similar to discovery cache and around half the size of the config table. Non-admin routes is only 200kb.
Sure, router could get a lot bigger, but every view that would be added there will also add a much bigger chunk to config.
Unsure how fancy we want to get with this, happy for some opinions.
- πΊπΈUnited States nicxvan
I think in 5 you meant this π Move route preloading into the UrlGenerator: don't simply preload on every HTML request Needs work
- First commit to issue fork.
- π¬π§United Kingdom catch
I tried to see how many routes we really have to load on most pages.
Made some small changes to @Berdir's MR to remove all caching, which is pushed to a draft MR.
Took these steps:
install standard
uninstall toolbar
install navigationLeft myself logged in as admin.
On a dynamic_page_cache hit, I get only get route lookups for the front page view route. This is for the is_front logic in preprocess + drupal settings. We could potentially cache that lookup independently in the bootstrap cache because it is done on every page. Would love to get rid of is_front altogether too but that's not easy unfortunately.
After truncating the dynamic page cache, I got , , the same views route, and bigpipe.no_js.
I think we could potentially handle the special and similar routes to circumvent the database/cache altogether too, not sure how yet but we never need to match against those routes, only load them.
Overall I think we could aim for zero route lookups when dynamic page cache is warm, one or two when it's cold. And then use @berdir's approach (without the serialization hack) to cache the individual routes.
- π¨πSwitzerland berdir Switzerland
I think I tested with disabled render caches, my concerns around the caching atm are how the impact is cold caches, agreed that warm caches should have very few lookups.
A variant of the caching idea above is that we could also only use the fast cache and just don't cache others. It should only happen in the backend and for admins, and it's essentially a 1:1 of a query lookup and cache lookup.
I pushed a proof of concept for that, still using the route preloader and a new method, didn't add to interface yet, route preloader is now a misleading name.
RouteProvider already implements events too. I wonder if we should just inline that cacheable routes stuff and make it an implementation detail, unsure.
- π¬π§United Kingdom catch
That seems like a good option. Prevents an explosion of stuff getting into APCu, because it's 1-1 cache items. Front end routes will only go in when they're looked up so it may not end up being all of them. Saves having to special-case the is_front handling.
- π¨πSwitzerland berdir Switzerland
Testing this a bit on our project, this removes all 5 or so preload cache lookups against the data bin without doing any queries on a dynamic page cache hit (2 from redirect, the other from toolbar).
On a dynamic page cache miss but render cache hits, I'm getting no query lookups either.
On pages with no/partial render cache, I'm getting a bunch of stuff from toolbar, not sure why that seems to be cached with different contexts like that, but different issue.
On admin/config, it results in around 26 queries against the router table, many due to the children access check stuff. But according to blackfire, that page executes around 2000 queries (1500 key value lookups), so I don't think that hurts too much. On other admin pages such as admin/content, local tasks and help and so on are getting cached and there are 2 or so non-cached router lookups.
- π¨πSwitzerland berdir Switzerland
One downside of this is that using bootstrap for this means that we it takes longer for the bootstrap cache to stabilize, every time a new route is discovered, a new bootstrap cache entry is written, last written updated and then the fast backend needs to be updated again. But I don't think it's worse than reading a new config or plugin discovery for a less frequently used plugin type or something like that.
- π¨πSwitzerland berdir Switzerland
Setting to needs review. there are test fails, but I'm looking for feedback on how to handle the RoutePreloader, if we should add a new interface on RouteProvider or just inline the whole thing. Depends on whether or not we want others to add more cacheable routes I suppose.
One thing is that we currently exclude for example json api routes, but we possibly might want to reconsider and maybe store routes per format so that we can inject other routes for the same format. not sure. This is where a json api performance test would come in handy, because I expect we'd see some regressions there as the routes aren't cached anymore.
- π¨πSwitzerland berdir Switzerland
berdir β changed the visibility of the branch no-caching to hidden.
- π¬π§United Kingdom catch
One thing is that we currently exclude for example json api routes
This logic is only used for URL generation, not lookup itself which always has to hit the database to path match, does that even happen for json api routes?
- π¨πSwitzerland berdir Switzerland
I didn't verify, that's why a test would be useful, but I think so? JSON:API contains lots of relationships and references to other pages, and I think they are created as routes?
- π¨πSwitzerland berdir Switzerland
Did a very basic test and I counted 10 or so router queries on a jsonapi route for a single article, they are now all uncached, on HEAD they are single data cache lookups.
The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.