Add PHP Fibers support to BigPipe

catch → credited mglaman → .

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.

The last submitted patch, 4: 3377570.patch, failed testing. View results →

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, but really we should just be asserting that the messages placeholder is last, it might need a bit more reworking.

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.

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().

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 big 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 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work 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 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, bigipe 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 an 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.

However, what if instead of locking and waiting, when we get a cache miss, we suspend the fibre we're in. If the parent process (bigpipe in this case) has another fiber to run, it can run that, and then by the time it comes back to us, one of the other requests might have built the theme registry for us.

CCF errors...

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

Is there a way/ideas about how to profile it and get edge cases when this concurrency can break things?

@andypost until we have 📌 [PP-1] Create the database driver for MySQLi Postponed 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 sets something on request or in session, and later code relies on this alread 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 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.

I just hit the infamous "cross-posting on a node can cause published = 0 to get set" bug on d.o 😳

#20:

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.

Agreed, this would be an existing bug 👍

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.

Blocks are not allowed to rely on global state, so that'd indeed also be an existing bug. (#lazy_builders' only dependencies are the arguments for the lazy builder callback — they can fetch information, but must not rely on request/global state. If they do, it's up to the developer to work around this. They probably should NOT use a lazy builder in this case.)

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.

Same! That last sentence is also exactly why they were able to add this to PHP without breaking anything, and while allowing much PHP code to be refactored in a simple way like we are doing in this very issue 🤩

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.

it could help largest contentful paint for example especially

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 😅

✨ Interface previews/skeleton screens through optional "preview" or "placeholder" templates Fixed (CR: https://www.drupal.org/node/3338948 → ) helps mitigate that, though, but we may need to do more issues like 🐛 Improve rendering account link in the toolbar Fixed .

In fact, I think doing this is a good motivation to actually prioritize doing more like that #2951268! 👍

I was very relieved to eventually not have to touch the BigPipe test at all

+1! 😄

it should be relatively straightforward to add a method + extra placeholder that does this.

Yep :)

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.

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.

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:

✨ [PP-1] Add async query execution support to views Active
📌 Use MessagesCommand in BigPipe to remove special casing of the messages placeholder Fixed

Unknown word (divpiggydiv)

The last submitted patch, 32: 3377570-30-test-only.patch, failed testing. View results →

#27: Yes, I've been thinking that too for a while now. 😇 But last time I looked into this (while getting CKEditor 5 ready for Drupal core and we needed to deal with AJAX updates and rendering the messages in a particular DOM location ), I was running into problems. Nothing insurmountable probably, but enough of an upstream blocker (it wasn't in core yet) to pursue an alternative solution. Definitely worth looking into though! 👍

#28: 👏
#29: 🤣 I totally approve of piggies! 🐽

Observations while stepping through, including a gotcha

To do the final review for this, I installed the big_pipe_test module and stepped through the entire rendering process. Observations:

1. I was surprised to not see <span>This little piggy stayed at home.</span> appear on the page 😅 Its placeholder remains unreplaced!
2. In stepping through it, $fibers was constructed based on $placeholder_order and looked like this:
   
   👆 This is the order in which the Fibers will be executed, time and time again, until none are left. Say that every Fiber gets suspended when they are started, except for #2 and #3. Those would only get executed when they are resumed (because of some I/O they're waiting for). That'd mean the execution order would be: #1, #2, #3, #4, #5, #6, #7, #8, #2, #3.

   This condition at the start of the Fiber processing loop guarantees that the messages placeholder is ALWAYS replaced last:

           if (isset($message_placeholder_id) && $placeholder_id === $message_placeholder_id && count($fibers) > 1) {
             continue;
           }
   

3. I can see it being executed in this order 👍
4. ⚠️ But as soon as we get to executing the Fiber for callback=%5CDrupal%5Cbig_pipe_test%5CBigPipeTestController%3A%3Aexception&args%5B0%5D=llamas&args%5B1%5D=suck&token=uhKFNfT4eF449_W-kDQX8E5z4yHyt0-nSHUlwaGAQeU placeholder,
   

     public static function exception() {
       throw new \Exception('You are not allowed to say llamas are not cool!');
     }
   

   is executed and … causes BigPipe's execution to STOP! 😱

5. Root cause:
   

         $fibers[$placeholder_id] = new \Fiber(function () use ($placeholder_id, $placeholder_render_array) {
            return $this->renderPlaceholder($placeholder_id, $placeholder_render_array);
         });
   

   🙈 NOPE I was wrong, it's because I'm in a core development setup and have configured system.logging:error_level to ERROR_REPORTING_DISPLAY_VERBOSE, rather than ERROR_REPORTING_HIDE which is what production sites use, and hence also what \Drupal\Tests\big_pipe\Functional\BigPipeTest::testBigPipe() uses 🙈

6. ALL IS WELL! 🤩👏

Code review

1. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
   @@ -536,73 +536,93 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
   +        if (isset($message_placeholder_id) && $placeholder_id === $message_placeholder_id && count($fibers) > 1) {
              continue;
            }
   

   🙏 It took me longer than I'd like to admit to figure out why the messages placeholder truly would ALWAYS be rendered last, even if a Fiber would keep getting suspended by its own #lazy_builder.

   THIS line is why. Obvious in hindsight. But less obvious while I'm still getting used to thinking in terms of Fibers' execution order 😅

   A comment here would go a long way 🙏

2. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
   @@ -127,6 +130,21 @@ public static function currentTime() {
   +   * #lazy_builder callback; builds <span> markup and suspends a Fiber.
   

   🤔 Wouldn't

   #lazy_builder callback; suspends its own execution, then builds <span> markup.
   

   be clearer?

3. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
   @@ -127,6 +130,21 @@ public static function currentTime() {
   +  public static function piggy() {
   

   Übernit: array return type 😇

4. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
   @@ -536,73 +536,93 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
   +            continue;
   

   Nit: this is at the very end of the loop already due to this refactoring, so this is a dead line of code 🪦

5. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
   @@ -127,6 +130,21 @@ public static function currentTime() {
   +    if (\Fiber::getCurrent() !== NULL) {
   +      \Fiber::suspend();
   +    }
   

   This could use a comment 🙏

6. +++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
   @@ -127,6 +130,21 @@ public static function currentTime() {
   +      '#markup' => '<span>This little piggy stayed at home.</span>',
   

   I feel VERY strongly that this is a missed opportunity for some piggy emojis; this is missing:

   • 🐷
   • 🐽
   • 🐖

   At least one, arguably all 😜

7. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -170,14 +170,18 @@ public function testBigPipe() {
   +      // The suspended placeholder is replaced after the non-suspended
   +      // placeholder even though it appears first in the page.
   

   🙏 Let's say why, at least with an @see?

Conclusion

As soon as some clarifying comments are added, I will RTBC this! 🥳 It's incredibly satisfying to see how easy it is to adopt new PHP language-level features! 😊

📌 Use MessagesCommand in BigPipe to remove special casing of the messages placeholder Fixed 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;
   +    }
   +
   

   Self review:

   Let's move this back to 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work 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.

The last submitted patch, 36: 3377570-36.patch, failed testing. View results →

The last submitted patch, 38: 3377570-39.patch, failed testing. View results →

#38 👍

No more concerns. This is well-documented, well-tested, and easy to understand. RTBC!

This is a quote I just have to share with y'all:

Amazing that Matt Glaman's blog post copy and paste hacks had one test failure to start with!

— @catch in Drupal Slack 😊

Thanks for getting this going, @mglaman! 👍

Wow, I still can't believe this turned into something. Amazing work @catch

+++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php
@@ -188,7 +189,63 @@ public static function cases(ContainerInterface $container = NULL, AccountInterf
+    $piggy->bigPipeNoJsPlaceholder = '<span data-big-pipe-nojs-placeholder-id="divpiggydiv</span>';

Looks like the span is missing the closing bracket. Is this intentional?

Nice catch, @marcelovani! 🦅👁️

That doesn't seem intentional, but a minor bug that the HTML parsers appear to be fixing automatically 😊 Still, we should get that fixed 🤓

Closing inline elements reminds about 🐛 Newline character in HtmlTag causes unwanted whitespace Needs work

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.

@catch I think you uploaded the wrong file

The last submitted patch, 49: 3377570-49.patch, failed testing. View results →

Test failure looks unrelated.

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

catch → credited Kingdutch → .

Update via 📌 Add Fibers support to Drupal\Core\Render\Renderer RTBC 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 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work .

Have a Umami 11.x running.
Applied the patch #56
Clicking around the front end no issues
Searching on the front end no issues
Went to modules and content pages and did searches no issues
Updated a view and verified I still get a status message just fine.

Not seeing any issues so think this is good!

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

Committed 108150c and pushed to 11.x. Thanks!

I've noticed that fiber loop itself is never suspended due to $iterations being reset at every iteration of the while loop. Pretty sure it was intended to be before the loop.

Also just random thoughts. For example, if we have 5-10 long-running fibers that just waiting for something, each foreach iteration will suspend the current fiber. So it can be very quick suspends in a row, if higher code has nothing to do we will be back here and again quickly suspended 5-10 times. Will it be better to move $iterations check to the while loop to minimize that "spin locking"?

@ReINFaTe can you please open a follow up. Will also need a failing test case to show this issue.

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.

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