Multiple load path aliases without the preload cache

Instead of doing this in the url generator we can probably do it in AliasManager::getAliasByPath() instead.

Add the system path to a class property ready for preloading, Fiber::suspend(), when we get resumed, look for any paths to multiple/preload that haven't already been, multiple load, continue.

The main question is how many times can we ::suspend() before we resume, but we only need to pick up two path aliases each time to reduce the number of queries by 50% so the odds might be decent. The existing solution has built in staleness where subsequent page requests will individually lookup paths that weren't found when the preload cache was set.

Put up a proof of concept here but it doesn't do anything useful on standard yet - almost nothing is rendered in a placeholder and that's the only inside-fiber rendering we do.

So this will need #3437499: Use placeholdering for more blocks - at least a rebase of the MR there and some combined testing, in order to see whether it can be viable.

I did add some views blocks (recent comments, recent content) to the footer along with adding some alias for things like comment/reply, and saw this working (i.e. it would multiple four aliases) - so in theory it has potential just not in practice yet.

Another attempt at a better issue title.

OK this actually works.

Manual testing with https://git.drupalcode.org/issue/drupal-3496369/-/tree/3496369-with-rend...

HEAD:

MR:

With a completely cold cache on an Umami recipe page, there are 38 calls to Drupal\path_alias\AliasRepository::lookupBySystemPath + the cache get and set for the preload cache.

With the MR, there are 31 calls to Drupal\path_alias\AliasRepository::preloadPathAlias and no cache get/set.

With a warm render cache, there will be zero path alias gets at all because the resultant URLs are in the render cache anyway.

So the worst case scenario is improved, and the best case scenario is the same.

When the path alias cache is warm but the render cache is not (multiple roles viewing the same path for example), then there will probably be more database queries - but what's in the cache will depend on which role visits the page first, so it's not like preloading gets the queries down to one, there's always some stragglers.

Then I added #3437499: Use placeholdering for more blocks locally and got down to 14 calls to preloadPathAlias() - this is nearly a 2/3rds reduction in path lookup queries on a cold cache. Because the cache is per-URL it will be a similar reduction across all the first visits to a URL on a site after a cache clear or each time the cache expires if the render cache for that page isn't warm."

This issue + #3437499: Use placeholdering for more blocks seems to be enough without #3496835: [PP-2] Render children in fibers, which is good because changing the renderer like that is a bit alarming.

However when testing I ran into the exception that #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter is trying to remove, so we will need that issue too to make this all work together.

The actual MR here still has a couple of test failures in path alias tests, but it'll be reviewable by itself without the other two issues - just won't be a performance improvement (or a very marginal one) until those also land.

Not exactly sure what test coverage looks like for this - we can probably set up some Fibers in a kernel test that call getAliasByPath() and check that the aliases are still returned correctly.

#3476421: Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache() would hopefully cover performance testing once it's in, alongside the other two issues.

However when testing I ran into the exception that #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter is trying to remove, so we will need that issue too to make this all work together.

Not entirely proven yet, but I think this might be ::executeInRenderContext() which relies on a (static) stack to track cacheability metadata from code that returns strings, some discussion in #3507959: Refactor the render context stack to avoid a static property.

I think we might need to do something like use a fiber in executeInRenderContext() so that we can 'trap' any fibers underneath - e.g. if they suspend, resume them again, so that any metadata doesn't leak into the stack.

Explicitly postponing this on #3437499: Use placeholdering for more blocks.

The is root exception is going to be a hard blocker here (and potentially for async queries too since I think it will hit the same thing).

Pretty sure this (and potentially any other fiber suspension within placeholder rendering) is blocked on #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter.

If you combine the changes here with #2511330: Deprecate RendererInterface::render()'s sole $is_root_call parameter you get a new error:

AssertionError: Bubbling failed. in assert() (line 634 of /var/www/html/core/lib/Drupal/Core/Render/Renderer.php) #0                    
                                         /var/www/html/core/lib/Drupal/Core/Render/Renderer.php(634):

But that gave me an idea on how to workaround it, pushing a new branch to see if it helps with the test failures.

MR up on #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers now. That makes this PP-2, but pretty sure those two issues are the only hard blockers and we mostly need to adjust test expectations here once they're in. Per #18 #3408713: Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest is necessary to see the improvement, although the existence of the test means we can run a combined MR to see the difference even if it's not actually committed yet.

With #3408713: Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest there is hardly any visible improvement or regression.

However manually testing with umami + the navigation module enabled, and then truncating cache_render, I do see a couple of path aliases loaded together (but several loaded individually).

If we were to render individual entities in views listings in placeholders (which we theoretically could), I think we'd see a significant saving though.

Removing 'try to' from the issue title because this definitely works.

Right now, with the three issues, it works, but only a couple of path aliases get multiple loaded.

To make it more effective, the following two issues would help a lot:

#3518662: Use a lazy builder/placeholder for the top bar
#3518668: Use Fibers for rendering views rows

The reason those should make a big difference is because the way this works is by picking up the first link it finds when rendering a placeholder, suspending the Fiber, and then doing that for each available placeholder. When it reaches the first placeholder again (e.g. the Fiber is resumed), then it will preload any path aliases picked up from the first iteration through the placeholders and load them at once. #3437499: Use placeholdering for more blocks did similar for several blocks already, but a lot of blocks don't necessarily have unique path aliases to look up.

So if you have five placeholders (at the same level of nesting), and each placeholder has one alias to load, you get five aliases loaded in one loookup.

But if you have a single placeholder with 50 URLs, then it will only multiple load the first one or two, then the rest individually, because it only has one placeholder with that many links in it.

For e.g. Umami views showing lists of recipes or taxonomy terms, if each of those is a placeholder, then we're looking at 10, 20, 50 path alias lookups at once instead of individually, and crucially, this will work on both render cache hits and misses. Also allows us to remove a high-cardinality, low-hit rate cache item at the same time. The entity/navigation placeholder issues make sense in their own right and could land either before or after this one, but it's another case where an improvement in one issue amplifies improvements in another.

Cross-linking the original issue that added the system lookup cache for multiple loading of path aliases back in 2009 #456824: drupal_lookup_path() speedup - cache system paths per page.. The code has moved around a lot but the logic hasn't really changed, pretty nice to be in a position to remove it with something better, umm, 16 years later.

In combination with the two blocking issues, and #3518668: Use Fibers for rendering views rows, and also #3408713: Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest, it's possible to see a real reduction in queries on cold caches now - about 15 less on the Umami front page and node pages. Combined MR is https://git.drupalcode.org/project/drupal/-/merge_requests/11784

    Open Telemetry Front Page Performance (Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformance)
     ✘ Front page performance
├ Failed asserting that two arrays are identical.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊  Array &0 [
       ┊ -····'QueryCount'·=>·376,
       ┊ +····'QueryCount'·=>·361,

 Open Telemetry Node Page Performance (Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformance)
     ✘ Node page
     ┐
       ├ Failed asserting that two arrays are identical.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊  Array &0 [
       ┊ -····'QueryCount'·=>·458,
       ┊ -····'CacheSetCount'·=>·441,
       ┊ +····'QueryCount'·=>·443,
       ┊ +····'CacheSetCount'·=>·440,

Combined MR with #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers is green now and removes about 500 lines of code.

To see a larger reduction in the number of database queries on cold caches, we need #3518662: Use a lazy builder/placeholder for the top bar and #3518668: Use Fibers for rendering views rows - but those can happen independently of this issue.

Will need a rebase here now the blocker is in.

The current MR only shows a very small reduction in queries in performance tests, this is because the specific URL aliases being rendered for anonymous users in those tests aren't in a good position to be preloaded.

However I opened an extra MR combined with #3518668: Use Fibers for rendering views rows (https://git.drupalcode.org/project/drupal/-/merge_requests/12470) which starts to show the potential on cold caches:

    $expected = [
-      'QueryCount' => 381,
 +     'QueryCount' => 366,
      'CacheGetCount' => 471,
  -    'CacheSetCount' => 467,
  +    'CacheSetCount' => 466,

The difference there is that all the URL aliases from all the views rows can be preloaded, so the Umami block listing taxonomy terms is able to get all the aliases for all the taxonomy term URLs at once instead of individually.

This should be ready for review now.

The draft MR at https://git.drupalcode.org/project/drupal/-/merge_requests/12470 is there to show the preloading working a bit better with one additional core issue applied, but shouldn't actually be reviewed.

Updated the issue summary.

This is ingenious, but what if there were another place in code where we wanted to do the same sort of trick? Is it the case that once we're doing this technique here, we can't do it anywhere else?

The answer here is... yes and no.

I've got #1237636: Lazy load multiple entities at a time using fibers open which works almost exactly the same as this, except for the fact that it breaks something horribly in placeholder rendering so does not actually work yet.

If we take the two together, you can see how it might or might not work, it will all depend on exactly what is going on with particular pages.

If you have multiple blocks that load one entity, then render a link to that entity, the process will look something like this:

Block 1 - entity 1 'preload' and suspend
Block 2 - entity 2 'preload' and suspend
Block 3 - entity 3 'preload' and suspend

Block 1 - loop returns and entity 1-3 are loaded - path alias 1 preload and suspend
Block 2  - path alias 2 preload and suspend
Block 3  - path alias 3 preload and suspend

Block 1 - loop returns, path aliases 1-3 preloaded, rendering finishes
Block 2 - rendering finishes
Block 3 - rendering finishes

So in the first scenario, three path aliases are loaded in one round trip, and three entities are loaded in one round trip, whereas prior to this change they'd each have been loaded one by one.

Overall 1/3rd the round trips.

But it could also look like this, in this case block 2 renders one path alias but doesn't load an entity either before or after.

Block 1 - entity 1 'preload' and suspend
Block 2 - path alias 1 'preload' and suspend
Block 3 - entity 3 'preload' and suspend

Block 1 - loop returns and entity 1 and 3 are loaded - path alias 2 preload and suspend
Block 2 - path alias 2 and 1 are loaded 
Block 3 - path alias 3 'preload' and suspend

Block 1 - rendering finishes
Block 2 - rendering finishes
Block 3 - path alias 3 loaded and rendering finishes

In this case, two entities are loaded in one round trip, three path aliases are loaded in two round trips - 3/5ths the round trips.

So because the paths and entities are out of sequence, it's not able to reduce the round trips as much - but the important thing is that it's still reducing them a bit, and the reduction isn't worse than if we only did path aliases or only did entities.

Also, if a single block/placeholder has multiple links in it, this approach only works when other blocks have the same number of links.

So if three blocks have one link each, three links will be loaded in one round trip. But if one block has three links, it will require three round trips - because the first link has to be loaded before the second.

There are situations where we have structures that are usually within a block, which predictably deal with multiple of the same thing, so we can make it work for those cases too:

#3518668: Use Fibers for rendering views rows would render views rows in fibers (even loop once we have that) and in general views rows are rendering exactly the same type of thing for each row (card view mode with an image and link, or a table row with links etc.)

Local tasks and similar places too where it's always a list of links. If we event-loop those, then we'll get a higher hit rate most of the time.

But also if two blocks have two links each, then those can be loaded in two round trips without any special handling withing the blocks (it works as long as the loop finds the aliases at the same point while it's progressively going through the blocks in the loop).

Overall, it does have limitations - we're never going to be able to load every single path alias on a page in a single round trip using this method, it will load 'batches' of path aliases that are rendered in similar code paths. But also the technique has almost zero overhead (apart from conceptually being quite complex, but the code itself is simple and lightweight), and the worst case is that it doesn't save much in particular scenarios.

This has been rebased since the bot marked it needs work.

#3518668: Use Fibers for rendering views rows is one such case where we can assume it's likely that multiple links/urls are involved. However we don't easily have a way to explicitly request preloading there or not that I can think of.

Another one which doesn't have an issue yet is local tasks/actions.

Those would definitely pick a lot more paths up and be less dependent on what's going on in the rest of the page.

There was a merge commit in the branch history which made things impossible to rebase. I started a new branch, reverted to the last known good commit, then rebased from there - have hidden the other branches for now.

Back to needs review. https://git.drupalcode.org/project/drupal/-/merge_requests/13252 is the one to review now.

Bot is understandably confused by the multiple branches. Closed all MRs except the active one.

Added an administrator performance test to Umami because I realised just being logged in as an admin is enough to show this working - it shaves 7 database queries off hitting a node page when all caches are empty. Pretty sure this is because contextual links are rendered via big pipe, and therefore happen inside fibers.

Test only job should hopefully show the diff https://git.drupalcode.org/issue/drupal-3496369/-/jobs/6679091

The important bit from those test failures is this:

-    'QueryCount' => 542,
-    'CacheGetCount' => 565,
+    'QueryCount' => 549,
+    'CacheGetCount' => 566,

e.g. this allows us to multiple load 7 path aliases on cold cache (Umami node page) requests, that previously had to be requested individually.

It also allows us to remove the per-page path alias cache - it doesn't work when it's cold, but also it doesn't do anything when render caches are warm because path aliases don't need to be resolved at all when the dynamic page cache / render cache can retrieve cached HTML.

This way we're getting an improvement on cold cache requests, no regression on warm caches, and by removing a per-page cache item we're also reducing overall cache storage requirements. There are some pages in the middle (mixture of warm and cold render caches) where it may result in more queries, but the current system doesn't perfectly deal with those anyway (e.g. if a non-admin warmed the path alias cache, an admin's path aliases have to be retrieved individually anyway) so even in those cases it could end up being neutral or an improvement.

The two threads on the MR should be resolved now.

In rebasing, it looks like once this issue is added (e.g. more fiber suspensions) there's an additional undiscovered bug with #1237636: Lazy load multiple entities at a time using fibers although fortunately one that was (I think) introduced by last minute refactoring which made it easy to find and would explain why it didn't surface before when the two issues were combined.

I've committed a fix here and will see if it gets tests back to green, but we might need to do a follow-up or revert and recommit in that issue or something.

Opened #3549380: Fiber entity multiple loading tweaks.

Cherry-picked the bugfix over here to try to get back to green in this issue.

#3549380: Fiber entity multiple loading tweaks needs to land before this one now, so explicitly marking postponed.

Rebased - only resolved merge conflicts / performance test changes since this was last RTBC, so I think I'm good to re-RTBC.

Discussed the removal of ::writeCache() with @alexpott and he suggested leaving it as a no-op with a deprecation just in case someone somewhere is calling it.