Add Fibers support to Drupal\Core\Render\Renderer

I have no idea if this works, just ran linting and one umami test, but something like this.

Should be better.

This looks like pattern so "fiber's loop" could be extracted to some trait or static method somehow

Wow! It feels like a whole new world of concurrency.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -161,6 +161,21 @@ public function renderPlain(&$elements) {
+  public function doRenderPlaceholder(&$placeholder_element) {

Can we add typehints?

This looks like pattern so "fiber's loop" could be extracted to some trait or static method somehow

Potentially yes but I'm not sure how - half the foreach loop is the same as the other patch, but half is doing fairly arbitrary things with arbitrary variables.

Can we add typehints?

We certainly can.

Also added $fiber->suspend() to an existing lazy_builder callback in the renderer unit tests.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

CS clean-up.

RendererInterface lies - renderPlain returns Markup or string.

Nice! 🤩

\Drupal\Core\Render\Renderer::renderPlaceholder() does 3 things:

1. Prepare: tweak the original render array to avoid re-auto-placeholdering
2. Render: the actual work
3. Store & bubble: store the rendered output in the render array and bubble up to the render context

I'm kinda worried about the new public doRenderPlaceholder() which does parts of the first and the second. The 3 different aspects are sprinkled throughout various parts: some in fiber creation, some in the new public method, some in the fiber processing. I'm hoping we can tighten that up a bit? (It took me a good while to grok what's going on because of this 🙈)

We can drop the helper and move it back inline - I added the helper to share code between renderPlaceholder and the fibers code, but it breaks the rule of 3 and is more code and indirection.

To get rid of the duplication we'd need to render the messages placeholder as a fiber, and in the process ::renderPlaceholder() would become unused, although still required by the interface. I don't know how to reconcile that bit.

Just posting the interdiff for discussion rather than a new patch for now.

Actually might have a better answer to #10, now renderPlaceholder() is three lines of code, with two protected helpers.

+  /**
+   * {@inheritdoc}
+   */
+  public function renderPlaceholder($placeholder, array $elements) {
+    // Get the render array for the given placeholder
+    $placeholder_element = $elements['#attached']['placeholders'][$placeholder];
+    $markup = $this->doRenderPlaceholder($placeholder_element);
+    return $this->doReplacePlaceholder($placeholder, $markup, $elements, $placeholder_element);
+  }

I like #12 much better! 🤩 Much easier to understand, because the original logic flow can still easily be seen:

1. public renderPlaceholder() now just calls 2 other things (doRender() + doReplace()) → public API surface unchanged 👍
2. protected replacePlaceholders() calls those 2 same things directly instead of calling renderPlaceholder(), to give it more control 👍

The most important caller of public renderPlaceholder() is BigPipe. This is fine, because BigPipe has its own parallellization.

Why both?

An obvious question is: do we need both?

This is my analysis/understanding, could you confirm its correctness, @catch? 🤓🙏

1. This issue makes \Drupal\Core\Render\Placeholder\SingleFlushStrategy faster, by rendering the different placeholders in parallel (in the sense that it's blocking I/O can happen in parallel), but still remains a single flush. Placeholders can be rendered in arbitrary order, but they must ALL be rendered for Renderer::renderPlaceholders() to finish (and of course, messages last).
2. 📌 Add PHP Fibers support to BigPipe RTBC makes \Drupal\big_pipe\Render\Placeholder\BigPipeStrategy faster, by rendering in parallel (same limitations as above) and streaming the fastest to render placeholder (least I/O-blocked) to the client first (but still guaranteeing that the messages placeholder will be rendered & streamed last).

The two issues are complementary: this issue makes all placeholders renderable in fibers and provides performance benefits (both back end and front end) even when BigPipe is disabled, whereas the other issue makes it possible to not have to tweak the placeholder render order: whichever one is least blocked on I/O will be served fastest (also both back end and front end, but only more front end performance benefits: just the same back end performance gain).

OK good that you like #12, I think it's the cleanest we can do. The split is necessary so we can have independent return values without trying to pass the same array to different fibers by reference, which I do not even want to find out if it works because it would be impossible to grasp even if it somehow does.

#13 is correct, I also opened 📌 Document why BigPipe no-js placeholder rendering doesn't use Fibers Active but to me that currently looks like the most work/least return (because we would render placeholders out of order, but still need to flush them in order, vs replacing as they come back as with bigpipe or waiting until they're all done as here). However we could do that and then it would cover every possible way that placeholders get replaced in core unless I've missed one.

And yes they're 100% complementary with no inter-dependencies. It all relies on 📌 [PP-1] Create the database driver for MySQLi Postponed and ✨ [PP-1] Add async query execution support to views Active (and perhaps 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work ) to get the actual performance benefit.

This is the amazing thing with fibers for me, the complete separation between 'I want to execute some code in a fiber because I potentially can run some other code if it suspends' and 'if I'm executing in a fiber, I'll suspend while some non-blocking i/o is happening to allow something else to happen' means we can add support in completely different places without conflicts or introducing interdependencies. Then it fits perfectly into Drupal's render API where at the very highest rendering level we end up with a set of placeholders which could have been nested deep inside entity/field/block/layout rendering pipelines, but end up getting processed all at once.

Note I also noticed 📌 Layout builder should use #lazy_builder to render blocks Closed: duplicate while looking at this stuff, which would benefit dynamic_page_cache and big_pipe with or without Fibers.

without trying to pass the same array to different fibers by reference, which I do not even want to find out if it works because it would be impossible to grasp even if it somehow does.

😱🤣 Yeah let's not!

#13 is correct, I also opened #3383521 […] looks like the most work/least return (because we would render placeholders out of order, but still need to flush them in order

Yeah I personally would lean to not doing that issue — or rather, change the scope documenting why it's not worth it 😇

This is the amazing thing with fibers for me, the complete separation […] means we can add support in completely different places without conflicts or introducing interdependencies.

💯💯💯💯 🥳

Then it fits perfectly into Drupal's render API where at the very highest rendering level we end up with a set of placeholders which could have been nested deep inside entity/field/block/layout rendering pipelines, but end up getting processed all at once.

All that hard work you, @FabianX and a few others did at the end of the Drupal 8 cycle is finally reaching its full potential 😊🤝

Discussed this and related patches with KingDutch today - mostly in the context of comparing the pure-fibers implementation here with libraries like amphp/revoltphp. That particular bit we are still discussing, there are features those libraries provide, but they are (so far) a few lines of code with pure-fibers, or harder to adopt (like amphp's async MySQL connection pool which would impose using a different database API to core's when you want to use it, or a fair bit of subclassing/decorating).

But we both realised one improvement can be made here on top of the current implementation.

Let's say we have a situation where four placeholders have executed four async queries and none have returned yet, so we're into our second iteration in the loop, we can end up in a situation where there's nothing else to do except 'spinlock' - going around the loop until one returns.

We can't perfectly detect that case, but we can start looking for other things to do in the case we get into the second iteration and things are still suspending. In this case, the renderer code can check if itself is in a fiber, suspend that fiber, and this would fall back to 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work in DrupalKernel::handle() (or something in-between if there was something) - widening the pool of things we can do while we wait.

See interdiff and patch.

This pattern can also be applied to the BigPipe implementation although I've not done that yet.

catch → credited Kingdutch → .

Wasn't entirely sure how to test.

Did the same thing as 📌 Add PHP Fibers support to BigPipe RTBC and applied the patch
Verified forms and messages still render just fine.

Since 📌 Add PHP Fibers support to BigPipe RTBC is RTBC going to RTBC this too!

The last submitted patch, 16: 3383449-16.patch, failed testing. View results →

Random test failure.

Committed d41f1a0 and pushed to 11.x. Thanks!

Found that this has the same issue as described here https://www.drupal.org/project/drupal/issues/3377570#comment-15271179 📌 Add PHP Fibers support to BigPipe RTBC

The fiber loop itself is never suspended due to $iterations being reset at every iteration of the while loop. Changed that in MR!5003

@ReINFaTe can you please open a follow up.

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

I had to change line 184 to

// Render the placeholder into markup.
$markup = $this->renderPlain($placeholder_element);
// return $markup;
return $markup ? $markup : '';
}

or it throws a "null given" error