Add PHP Fibers support to BigPipe

This is @mglaman's blog post code snippets in patch form. BigPipeTest fails for me locally with head, rather than debugging that on a Friday evening (it is probably apache-specific and I'm running nginx with ddev), let's see what the bot says.

This might be a green patch - at least BigPipeTest now passes, and that was the only failure, so as long as it doesn't cause another test to fail.

I don't think this is 100% yet but it's definitely getting closer.

Things to look at:

1. We need to preserve artificially rendering the messages placeholder last - because it relies on messages added to the session which might be set by other placeholders. This logic is a bit janky already and let's say the way I handled it does not make it less janky by any means.

Also the test passes if you remove the start and stop signals from the array (see below), but really we should just be asserting that the messages placeholder is rendered last, it might need a bit more reworking to test only the thing we actually care about here.

2. We can't check for the existence of only one start and stop signal, because now we're sending one per placeholder instead of one per set of placeholders. While BigPipe is designed to accommodate this (i.e. rendering one group of placeholders, then another group of placeholders later potentially), it makes me wonder if we should try to refactor things a bit so that we continue to render the group of placeholders inside a single start and stop signal. This would probably mean moving some logic in renderPlaceholders() into another helper method, and making that method what we wrap into a Fiber - something like this. However better to start from a working patch before embarking on bigger changes.

3. We can't eat exceptions from fibers, we need to just let them be thrown. BigPipe itself already has code to convert exceptions to E_USER_ERROR and fortunately test coverage to make sure it works.

Tagging for subsystem maintainer review since this could really use a look from Wim and/or Fabian.

As well as views listing queries and entity queries, any other potentially long query that we do during rendering is a potential candidate for async mysql queries + fibers.

Drupal\path_alias\AliasWhiteList::resolveCacheMiss() would be one - we could check if we're in a fiber when we reach that (somewhat likely because we'd be rendering a URL, which can happen in a bigpipe placeholder), if we are, execute the query async, Fiber::suspend() etc.. Since this happens for each unique first part of a path (node/, user/, admin/ etc.) there's a fair bit of potential there.

it makes me wonder if we should try to refactor things a bit so that we continue to render the group of placeholders inside a single start and stop signal.

I did this, now get a green test run (on BigPipeTest at least) with no test changes, and while the diff is larger due to indentation, the actual functional change is considerably smaller - i.e. we only send one start and stop signal, only create the fake request once, stuff like that.

I wondered if we could skip the placeholder ordering altogether and only detect the messages placeholder ID and render it last, however even though Fibers means that the placeholders can get sent out of order, I think we still want to start them in DOM order - the quicker they start, the quicker they can finish, and might as well render top down if and when we can, so that logic is fine as it is.

Consolidating three identical try/catch statements down to one, HEAD currently has two, adding the third made me realise we can just move them a bit higher.

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -536,74 +536,91 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
+          }
+          elseif ($fiber->isTerminated()) {
+            $elements = $fiber->getReturn();
+            unset($fibers[$placeholder_id]);

This shouldn't be an elseif.

If the fiber doesn't call suspend() at all, or if it was already suspended earlier and doesn't call suspend() again, it will either throw an exception or get all the way to termination in one go. So either the start() or resume() calls could mean that by the time we get here, it's already done and we should send the response immediately.

With the elseif, we'd need to go all around the foreach loop another time before we send.

Let's be optimistic :)

Getting rid of one level of indentation.

Failed to remove the now-unnecessary test changes from the patch.

I think we can make use of this in core even before we have async database queries. Without async queries it would only be an improvement on cold cache requests to high traffic sites (i.e. a stampede situation), but it also shows a way we can optimise things anticipating async queries too.

Ideas are from #3257725: Add a cache prewarm API but including the changes here because I think it's more appropriate to this one, the other issue would then be able to build on top of the stuff added here.

Let's say we have two requests coming in to a site within 200ms of each other, and let's assume for the sake of argument that building the theme registry takes one second and they're both requesting HTML pages.

If both requests get a cache miss for the theme registry during bigpipe placeholder rendering, then they will both try to build it at the same time, taking a second each. An approach we've taken with the router is to add a lock and lock wait to ensure only one request does this at a time, this means all the other requests wait around doing nothing until one has done the work, then they can all proceed.

However, instead of waiting, if we suspend the fiber, then the parent process can move onto something else, in the case of this issue rendering a different bigpipe placeholder - this could involve running a non-async database query, or an async database query, or building some other cache, or loading an entity, or anything which takes a few milliseconds or more.

Let's say request 1 only has one bigpipe placeholder, and it gets a cache miss for the theme registry. It suspends the fiber, but because there's only one fiber, bigpipe immediately resumes it again, with the change in this patch, it will check the cache again just in case, then move onto building and caching the theme registry.

Request 2 has two placeholders, the first one immediately hits the empty theme registry cache too (because it's just showing the current user's name or similar) and calls Fiber::suspend(), bigpipe moves to the next fiber, which is a views block which does a listing query. It runs the query and then eventually needs to render the results, this arrives at the theme registry again, because request 1 has already finished building it, it gets a cache hit. Once the views block is finished, the first (username rendering) placeholder is resumed, and this also gets a cache hit for the theme registry now. Now without either locking or waiting or async, request 2 was able to get on with something else while request 1 was building the theme registry for it and it skips having to do that work itself.

With async queries this will help the single-request case too.

Let's assume the same two-placeholder page with a username and a views block.

The first placeholder renders the username, it gets a cache miss for the theme registry and suspends the fiber.

The second placeholder is rendering the views listing, it executes an async listing query, and suspends the fiber.

Bigpipe now resumes the fiber for the first placeholder, because it got a cache miss before, the registry double checks that's still the case, then goes ahead and builds the theme registry because it's still not there.

Once the first placeholder finishes, bigpipe then resumes the second placeholder, the query has already come back, so it keeps going.

If we didn't call Fiber::suspend() from the theme registry cache miss, then the first placeholder would just continue all the way through until finishing, and then the second placeholder would just have to wait for its async query to come back after being immediately resumed - because there's no other placeholder for bigpipe to execute while it waits.

So suspending when we get to a CPU-expensive cache miss allows us to move on to the next thing, and if the next thing is async, then we return to the CPU-intensive task while the async thing is running, or if we're lucky (if you can call stampedes lucky) don't have to do it at all. This massively increases the likelihood that we're able to take advantage of things happening in parallel, whether it's due to async or different requests coming in.

Also adding similar logic to LockBackendAbstract::wait() for all the same reasons, see the comments in there.

CCF errors...

And we need an early return if the first cache get is successful.

@andypost until we have #3259709: Create the database driver for MySQLi and views support for async queries in ::execute(), it won't improve anything at all for the single request case. We could profile to make sure it doesn't make anything worse, which @mglaman already did.

It would be possible to set up a test situation using guzzle async requests though - i.e. have six different blocks that are all placeholdered, and all make an async guzzle request to somewhere, it would then call Fiber::suspend() after the request is made and before waiting for it to come back. If that async request was to a route that adds a sleep(5) call before returning, it would make things more obvious.

We should then be able to see that bigpipe loops through the placeholders, and they all send the async guzzle request, then it loops through them until they start coming back and renders them in whatever order they come in, instead of doing each one by one.

The main edge case I can think of is something like this:
Someone adds a footer block which adds a library and is placeholdered. The library has an implicit dependency on a menu library but the dependency is undeclared because it's always rendered last after the menus at the top of the page anyway. This change means the footer sometimes gets rendered before the header, so its library is added in the wrong order. However this would be exposing an existing bug (the lack of a library dependency) rather than introducing a new one.

Another case would be some code running inside a placeholder sets something on request or in session, and later code relies on this already having been set, but now they're rendered out of order so it's not set - this is what we already have to account for with the messages block. But again this would be a pre-existing bug - the block might be rendered on a page where the other block never appears for example.

It looks me a while to grasp conceptually what Fibers does, but that's partly because it's deceptively simple - i.e. you can start some code, and pause some code, and restart some code from different places, and that's about it. Nothing actually runs at the same time, except non-blocking i/o.

Now with Fiber support, there effectively is even *no* way to control the execution order — previously you had implicit control: the DOM order.

This is true but we still control the *start* order if not the *finish*/render order, updating the method docs on ::getPlaceholderOrder() instead of this inline comment sounds good. I originally thought we should get rid of getPlaceholderOrder() entirely, but I think it's fine and probably good to start in DOM order, it could help largest contentful paint for example especially if the first placeholders are quick to render and don't call Fiber::suspend().

We can do a benchmark and a test then, and they can be one and the same: by having an artificially slow block's #lazy_builder call Fiber::suspend() explicitly and then asserting that it gets rendered last despite being NOT the last block in the DOM order.

Yes I think this will work for a test. I was very relieved to eventually not have to touch the BigPipe test at all, but from what I saw when I thought I would have to, it should be relatively straightforward to add a method + extra placeholder that does this.

I think all the other remarks are good - I'll try to address them soon. On fiber vs. Fiber I can't decide, but probably it's easier to use Fiber since its a proper noun in PHP now I guess.

This is my one remaining concern about this change: it's possible that this change will cause a worse end-user experience, because due to async rendering it could be that the most visible block is now rendered last 😅

Yeah it's going to depend on the request, with warm render caches there should be zero change. In a cold cache situation it could result in a later LCP and especially more layout shift, but other times it'll be less (say if the LCP is actually the third placeholder and this ends up getting rendered first, which could be the case with main + user menus and similar). Adding more previews will definitely help to mitigate the times when we're unlucky. But also I would think if the overall page renders quicker it should help things like time to interactive more or less across the board.

I've addressed everything (I hope) except the additional test coverage.

Also I left the Drupal 8 comment as is when moving the messages code around, but when I saw it still said Drupal 8, I wondered if anything's changed, and maybe we can use messages command? Render the placeholder, Drupal::messenger()->getMessages() or whatever it's called now, shove the messages into a MessagesCommand. Definitely follow-up material but could be nice. There's a good change messages are the LCP on pages that have them so it could be quite significant (albeit on a tiny proportion of requests).

Here's a bit of extra test coverage - adds a Fiber::suspend() to a callback that sends a message, to ensure that messages really are rendered last even if we go over the loop twice.

If I hack out the fiber-specific messages handling like this:

diff --git a/core/modules/big_pipe/src/Render/BigPipe.php b/core/modules/big_pipe/src/Render/BigPipe.php
index a22d7a633e..de71eaf26f 100644
--- a/core/modules/big_pipe/src/Render/BigPipe.php
+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -557,9 +557,6 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
     }
     while (count($fibers) > 0) {
       foreach ($fibers as $placeholder_id => $fiber) {
-        if (isset($message_placeholder_id) && $placeholder_id === $message_placeholder_id && count($fibers) > 1) {
-          continue;
-        }
         try {
           if (!$fiber->isStarted()) {
             $fiber->start();

Then I get this test failure:

1) Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testMessages_2712935
Behat\Mink\Exception\ExpectationException: The string "<div role="contentinfo" aria-label="Status message"" was not found anywhere in the HTML response of the current page.

So that's good - it's testing what we want.

I started to look at BigPipePlaceholderTestCases and the bigpipe_test controller for trying to add an extra case there to assert things are rendered out of order, and didn't get very far at all yet though.

Here we go. I'm not sure Wim will approve of the species introduction but all seems to be working.

Test only patch is also the interdiff.

And the actual patch.

Follow-ups:

#3379883: [PP-1] Add async query execution support to views
#3379885: Use MessagesCommand in BigPipe to remove special casing of the messages placeholder

Unknown word (divpiggydiv)

#3379885: Use MessagesCommand in BigPipe to remove special casing of the messages placeholder looks like it has legs which would remove this tricky line altogether, but I will add the clarifying comment for #1 in the meantime.

Everything else looks good - will try to address soon, gonna find out if vim will handle porcine emojis.

Should address all of #34.

Self review:

1. +++ b/core/lib/Drupal/Core/Lock/LockBackendAbstract.php
   @@ -37,18 +37,30 @@ public function wait($name, $delay = 30) {
    
   +    // If executing inside a fiber, then suspending the fiber implicitly waits
   +    // for whatever the parent process does before it is resumed again. Try
   +    // that so that other tasks can continue, before sleeping only if necessary.
   +    if (\Fiber::getCurrent() !== NULL) {
   +      $sleep = 0;
   +      \Fiber::suspend();
   +    }
   +    else {
   +      // Begin sleeping at 25ms.
   +      $sleep = 25000;
   +    }
   +
   

   Let's move this back to #3257725: Add a cache prewarm API where is is more closely tied, it's been reviewed and already changed there. Just removed the hunk from the patch.

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -237,22 +237,46 @@ protected function init($theme_name = NULL) {
   -        $this->setCache();
   +    // If called from inside a fiber, suspend it, this may allow another code
   +    // path to begin an asynchronous operation before we do the CPU-intensive
   +    // task of building the theme registry.
   

   On the other hand the theme registry change is more relevant here, since that becomes blocking when we're trying to render stuff, slightly improved the comments though.

Yes that's a mistake I am afk for a bit, but also one character is fixable on commit if no-one else gets to it.

Addressing #45.

Test failure looks unrelated.

Opened #3383449: Add Fibers support to Drupal\Core\Render\Renderer - no dependencies either way with this issue, just applying the same overall pattern.

Update via #3383449: Add Fibers support to Drupal\Core\Render\Renderer and discussion both these issues and others with @Kingdutch - once we've gone around the fibers loop once, if they're still waiting, we can allow calling code to do something else if it wants to too. This reduces the likelihood of a spinlock in the case something async is taking a very long time to come back, since we can do something else useful in the meantime.

The core use case would be #3257725: Add a cache prewarm API.

Converted to an MR and switched from longhand function () use($foo) syntax to an arrow function.

The $iterations thing is an optimization, it's not testable except perhaps manually but even that would be very difficult. Let's discuss the rest of #64 on the follow-up issue, I think the fix itself looks correct, we might want two follow-ups, one for the quick fix and one for the spinlock discussion.