Use Mutation observer for BigPipe replacements

That looks interesting :)

Developpement happens on the latest branch which is 9.2.x these days, there shouldn't be any new features on the 8.x branch.

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -518,24 +502,6 @@ protected function sendNoJsPlaceholders($html, $no_js_placeholders, AttachedAsse
-    // Send the start signal.
-    $this->sendChunk("\n" . static::START_SIGNAL . "\n");

@@ -556,58 +522,75 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
-    // Send the stop signal.
-    $this->sendChunk("\n" . static::STOP_SIGNAL . "\n");

Is it correct changes?

** Disclaimer: I don't truly understand how big pipe works (PHP side mainly)**

#7,
From my understanding, the current approach uses `setTimeout` in bigPipeProcess to process the new elements, not the signal?

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -1,133 +1,79 @@
-        const ajaxObject = Drupal.ajax({
-          url: '',

current bigPipe's `Drupal.ajax` doing a simple DOM job, not the networking. (plus potential problems: https://www.drupal.org/project/drupal/issues/1988968)

Is the SIGNAL a bug (or useless) all time?

#8,
I believe this patch's trick is MutationObserver, not the TEMPLATE tag. In bad cases, we can take the current response format (with mods).

@nod_,

Just interested why we don't use `MutationObserver` before and find you:
https://www.drupal.org/project/drupal/issues/2469431#comment-10471584

5 years later mutation observer support is good on all supported browsers so we can go that way now :)

The script tags used to write the data from bigpipe is wrapped in a script type that has a special type. The code inside is considered a "data block" and not parsed by the browser so it's doing the same thing as a template.

Thanks, there are a bit too many changes here. not only changing what's triggering the processing happens but also how things are processed.

1. +++ b/core/modules/big_pipe/big_pipe.libraries.yml
   @@ -5,7 +5,5 @@ big_pipe:
   -    - core/jquery
   -    - core/jquery.once
   

   We still need the .once library

2. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   -  function bigPipeProcessPlaceholderReplacement(index, placeholderReplacement) {
   ...
   +  const processReplacement = (replacement) => {
   ...
   -      // If we try to parse the content too early (when the JSON containing Ajax
   -      // commands is still arriving), textContent will be empty or incomplete.
   

   So because we use mutation observer we won't run into the issue outlined below in the function, but let's get the patch green before trying to change that one. Let's keep this function as-is for now and see if can improve it once the rest is ok.

3. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   +  const processNodes = (nodes) => {
   +    forEach(nodes, (node) => {
   +      if (node instanceof HTMLScriptElement && node.hasAttribute('data-big-pipe-replacement-for-placeholder-with-id')) {
   +        processReplacement(node);
   

   I don't see the value of separating this function, can we put that directly in processMutations?

4. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   +  // IE does not support NodeList.forEach().
   

   We have a polyfill for that, add core/drupal.nodelist.foreach in the dependencies and you won't need this.

5. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   +  const forEach = (nodes, callback) => {
   ...
   +  const processMutations = (mutations) => {
   

   Here and in the rest of the file why not use functions? If it's for the character count, it's not shorter:

   function toto() {}
   const toto = () => {};
   

6. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   +      if (node instanceof HTMLScriptElement && node.hasAttribute('data-big-pipe-replacement-for-placeholder-with-id')) {
   

   a bit verbose, we can use node.matches('script[data-big-pipe-replacement-for-placeholder-with-id]')

7. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,131 +3,84 @@
   -  $(window).on('load', () => {
   ...
   +  window.addEventListener('DOMContentLoaded', () => {
   

   The cleanup happened at window load, any particular reason to change this?

And there are a bunch of code standards errors, make sure that yarn run lint:js-core-passing is clear.

core/modules/big_pipe/js/big_pipe.es6.js
  16:41  error  Replace `'data-big-pipe-replacement-for-placeholder-with-id'` with `⏎······'data-big-pipe-replacement-for-placeholder-with-id',⏎····`                                                                                                                             prettier/prettier
  49:5   error  Expected an assignment or function call and instead saw an expression                                                                                                                                                                                             no-unused-expressions
  56:11  error  Replace `node·instanceof·HTMLScriptElement·&&·node.hasAttribute('data-big-pipe-replacement-for-placeholder-with-id')` with `⏎········node·instanceof·HTMLScriptElement·&&⏎········node.hasAttribute('data-big-pipe-replacement-for-placeholder-with-id')⏎······`  prettier/prettier
  64:40  error  Insert `;`                                                                                                                                                                                                                                                        prettier/prettier
  73:11  error  Replace `document.querySelectorAll('[data-big-pipe-replacement]'),·processReplacement` with `⏎····document.querySelectorAll('[data-big-pipe-replacement]'),⏎····processReplacement,⏎··`                                                                           prettier/prettier
  77:20  error  Insert `,`                                                                                                                                                                                                                                                        prettier/prettier

✖ 6 problems (6 errors, 0 warnings)
  5 errors and 0 warnings potentially fixable with the `--fix` option.

this patch reminded me (give me confidence) that my suggestion below could take one more step to remove the `setTimeout` (MutationObserver or any monitors). Just do a simple pair the execute the command with the responses

https://www.drupal.org/project/drupal/issues/2903614#comment-13717905

removed setTimeout:
https://git.drupalcode.org/issue/drupal-2903614/-/compare/9.2.x...290361...

** and @casey is right! Probably we can remove the start/stop SIGNAL

Pretty neat, it makes it a bit harder for CSP #2513356: Add a default CSP and clickjacking defence and minimal API for CSP to core but not impossible. Not 100% sure which solution to go with.

#14 is nice :) I like the simplicity of it. On the other hand, as a maintainer inline JS makes CSP harder (not impossible, just harder) and it adds JS code inside PHP strings, which makes things harder to keep track of. As much as I like the solution, I would rather not add JS-in-PHP code in core.

Patch is looking better :)

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -30,104 +33,70 @@
+  window.addEventListener('DOMContentLoaded', () => {

Still worried about that one, I'm thinking async script could be an issue somehow.
then again core support for async is pretty poor.

I don't want to remove the start/stop signals yet, it might be a BC break, i'll check first.

Can you update the issue summary with the fact that we're removing the direct jquery dependency as well as using mutation observer? Thanks!

@nod_

We are always able to tweak it. I always prefer a fast workaround first if it won't bring extra BC noises.

(Technically, MutationObserver will resolve the race conditions problems. Not truly revolved but the probability will become verrrrrrrry low. I will perform some basic tests soon)
(https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/)

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -18,62 +21,48 @@
+        if (node.nodeType === Node.ELEMENT_NODE && node.matches(replacementsSelector)) {

`node.nodeName === "SCRIPT"` can filiter out more unrelated before more expensive `matches`

`DOMContentLoaded` is better than `load`.

BUT, we may have to use CORE's domReady instead.

we can remove the start/stop signals from the html, just need a change record.

#3204273: Remove jQuery dependency from BigPipe is in
needs reroll

🤞 Would be cool to move this forward again indeed!

re-roll of #30

could we do this instead?

drupalSettings.processMutationsCallback = drupalSettings.processMutationsCallback || (function processMutations() {});
window.addEventListener('DOMContentLoaded', drupalSettings.processMutationsCallback)

this provided an easier way to swap it.

** I preferred it under `window.Drupal` than `drupalSettings` if we can.

@droplet do you mean kinda that?

fix cs, interdiff vs #44

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -94,9 +94,15 @@
+    drupalSettings.processMutationsCallback ||

should be
`Drupal.bigPipe.processMutationsCallback = Drupal.bigPipe.processMutationsCallback || function processMutationsCallback() {}`

Fixed

@droplet in what situation would you override the domcontentloaded callback? I'm not sure what would be the advantage of allowing to swap those callbacks.

Need to deprecate the setting:

drupalSettings.bigPipeInterval

@droplet, i would understand if what we can override is the processMutations or processReplacement functions since it could change how we select elements to process and change the way the replacements are processed but I'm not sure about the load callback.

I mean the load callback should probably be a behavior no?

Need to deprecate the setting:

Nice — it'll be deprecated and have zero effect, because the interval basically becomes 0, always, since it'll always be instantaneous thanks to this much better browser API! 🥳

Loving this improvement!

1. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -30,103 +33,76 @@
   +    // Create a Drupal.Ajax object without associating an element, a
   +    // progress indicator or a URL.
   

   Übernit: 80 cols: progress would now fit on the first line 🤓

2. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -30,103 +33,76 @@
   +    // Then, simulate an AJAX response having arrived, and let the Ajax
   +    // system handle it.
   

   Same thing here.

3. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -30,103 +33,76 @@
   +  function processMutations(mutations) {
   +    mutations.forEach((mutation) => {
   +      mutation.addedNodes.forEach((node) => {
   +        if (
   +          node.nodeType === Node.ELEMENT_NODE &&
   +          node.matches(replacementsSelector)
   +        ) {
   +          processReplacement(node);
   +        }
   +      });
   +    });
   

   🤩

Addressing nitpicks mentioned in #52

Addressed nitpicks mentioned in #52 & also added an interdiff between #49 & #54

tried the #54 patch on the 9.3.x setup and caught this error:
Uncaught TypeError: Cannot read properties of undefined (reading 'processMutationsCallback')
tested on chrome:94.0.4606.61

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -18,62 +21,51 @@
+  Drupal.bigPipe.processMutationsCallback = Drupal.bigPipe.processMutationsCallback || function processMutationsCallback() {

This line throw the error

Drupal.bigPipe is undefined at that line, so it has to be initialized

@voleger that's very strange to me because it works for me in FF and chrome but in console
Uncaught TypeError: Drupal.bigPipe is undefined

I'm wondering why the test suite does not track the console for thrown js errors?

I'm wondering why the test suite does not track the console for thrown js errors?

Reminds me of #3228956: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests.

i would understand if what we can override is the processMutations

You're right. I overlooked how it works. Then it may not be worth making it optional. (because replace the whole file is better)

Since we don't need to make the implementation switchable, Can we go back to the code of patch #44 with the fixes from #52 applied?

Thanks, almost there :)

object Drupal.bigPipe is NOT existing at all!

should to be something like
Drupal.bigPipeProcessMutationsCallback = Drupal.bigPipeProcessMutationsCallback || ...
...
window.addEventListener('DOMContentLoaded', Drupal.bigPipeProcessMutationsCallback);

I think it's ok to keep it replaceable, just no reason in extra object in Drupal object

Interdiff vs #54

If we make things replacable it should be processMutations, not the processMutationsCallback function, it's not clear to me what the benefit would be to make this one pluggable, do you have a use case for this?

Still think we should go with #44.

I can't find other case except de-touch of listener for having a name

patch as #62 suggests

btw it can win few more bytes in size via #65 - thank you!

is it me or does this new code makes the CPU work extra hard compared to the previous implementation?

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -18,62 +21,48 @@
+    mutations.forEach(function (mutation) {
+      mutation.addedNodes.forEach(function (node) {
+        if (node.nodeType === Node.ELEMENT_NODE && node.matches(replacementsSelector)) {

Is there a more efficient way of checking the node other than .matches? We know it must be a script tag with a specific attribute?

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -18,62 +21,48 @@
+  observer.observe(document.body, {

Do we have to observe the entire body and all children, can we restrict this somehow? I guess one issue is that when we process the replacements, the mutation observer is being fired again because we are adding more nodes to the DOM.

Can we set subtree: false? We know where the nodes will be added, we don't have to observe the entire tree?

edit: more suggestions here that might help: https://stackoverflow.com/a/39332340

We should try #32

@nod_

Is the extra CPU time unacceptable? or just more. New implementation must use more CPU.

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -30,103 +33,70 @@
+    replacement.parentNode.removeChild(replacement);

this line is unnecessary in new approach. Or we should update them to something (eg. remove attr) that won't trigger MutationObserver childList's update.

If we see bigpipe process as one whole thing, removing them after observer.disconnect(); is better.

To me it's unacceptable at the moment. Very good point about not removing element right away

All right, chaged a few things, looks like my cpu is happy now.

I didn't remove the scripts from the DOM because we didn't do that before either. not sure of the benefit of removing them.

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -73,9 +73,11 @@
+          requestAnimationFrame(() => processReplacement(node));

used that because the DOM is changing so it makes sense to do things before rendering. setTimeout or queueMicrotask works too.

@nod_

The changes make sense but I wonder about the limits. Could you drop a counter to see how many replacements are on your test env? I bet your theme has a complicated layout that triggers CSS reflow..etc. Or triggered Drupal.attachBehaviors ?
(https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/misc/ajax.es...)

And what's your CPU model (if you don't mind I asked)

We are able to take off observer.disconnect(); and code a simple script to queue 10, 20, 30, 50 fake replacements per time and see what happens.

and IE11 testing if it is still supported. (and slow Android phone?)

(The reason I asked optional callback before is I want to change the rendering timing for some cases)

There are 4 replacements on most pages, and my CPU is getting old: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz. I'm using an empty drupal site with olivero and claro enabled. I'm mainly going through the admin UI like the content type field UI, menu UI, etc.

I think the nodename check, removing the matches and not removing the script element was the main reason for the improvement. calling directly processReplacement worked well too, it didn't have visible impact on performance. Using requestAnimationFrame was more of a "just in case" thing.

#67

is it me or does this new code makes the CPU work extra hard compared to the previous implementation?

Ehhh… 😬😨. This is why I wrote Nice — it'll be deprecated and have zero effect, because the interval basically becomes 0, always, since it'll always be instantaneous thanks to this much better browser API! 🥳 in #52, but apparently that's not true?

#73–#76: any chance that the DOM changes made by the removal of the <script> tags previously actually triggered more mutation events? In any case, I'm happy to hear that the performance regression is now gone.

We should also test this on low-power devices. BigPipe went through a very long, very cautious release process into Drupal 8's standard install profile (see https://wimleers.com/blog/drupal-8.3-bigpipe-module-stable), to minimize disruption risk. We should be at least equally mindful now.

The change of removeChild reduced below replacement execution time from 4ms to 2ms. It's a lot.

data-big-pipe-replacement-for-placeholder-with-id="callback=shortcut.lazy_builders%3AlazyLinks&&token=XXX

However, I can't figure out the base problem because all other 3 replacements have no difference.

And I noticed if I'm testing it in normal browser env with adblockers (, and other plugins), the execution time increased to 7ms. (I'm surprised.)

My guess is MutationObserver moving the execution timing to loading or interactive. At that moment, browser is heavy loading. (near to FCP & LCP, peak CPU loading)

Then, we have another further tweak:

With MutationObserver, Drupal.attachBehaviors could be called before the standard one from drupal.js

We able to intrdocue a global flag window.Drupal.isGlobalAttachBehaviorsFired and skip them.
(Needs TRUE by default if we allow exception for other scripts window.Drupal.allowFiredBeforeGlobalAttachBehaviors = true)

** same to Drupal.detachBehaviors
** not-related issue: scripts from comment editor trigger 35 times of Drupal.attachBehaviors for non-Aggregate JavaScript files. 3 times for Aggregate enabled.
**
** also, I noticed non-sense executions time on $newContent.each in Drupal.AjaxCommands.prototype.insert. Needs try to replace $.each and profile again.

Then, we have another further tweak:

I think that means this needs some work? :)

I think we might need to keep the timing concept and only use the mutation observer to fill in a queue of things that needs to be processed. This would remove some uncertainties (no need for the try/catch around json.parse) while keeping the performance profile.
Or maybe do a debounce type of thing and start processing elements when things are idle for a few hundred miliseconds.

This would remove some uncertainties (no need for the try/catch around json.parse)

Is it just a concept (theory)? You can't query the SCRIPT if there's no closing tag before DOMContentLoaded event

Otherwise, you can flush a START tag <div id="bigpipe"><script type="application/vnd.drupal-ajax" data-big-pipe-event="start"> and reduce the scope to

observer.observe(document.querySelector('#bigpipe'), { });

(I'm sure it doesn't work. I have tested it before)

Replace setTimeout to requestAnimationFrame is another option. I don't think extra calls is a performance problem.

Or maybe do a debounce type of thing and start processing elements when things are idle for a few hundred miliseconds.

It doesn't right. We need it as FAST as possible.

** Further tweaks can do later on a new issue. Current code on slow response has the same problem

the related for #81

Twice runned drupal environment without/with patch #73
- new article /node/1 with one comment. Caching enabled. Olivero with admin bar.

on chrome lighthouse:

before patch: https://gyazo.com/261803aef558db58a5a0ce3cb44e20ff
after: https://gyazo.com/e2d66c4097fc5bbcc31dcbfde9de937d

Largest Contentful Paint became much better after patch.
Cumulative Layout Shift became less dramatical.

The improvement is more then x2 interesting how it depends on platform+browser

But anyway impressive

#85: wow! Thank you! Bumping to Major based on that!

@nod_ I think we should try to finish this 🤓

I marked this Needs work in #79 based on @droplet's review in #78. But in #81 droplet said Further tweaks can do later on a new issue. Current code on slow response has the same problem → let's move this back to Needs review, and let's first hear back from @nod_ what he thinks should still be done 😊

Ok so the numbers are better with mutation observers indeed, we remove 2 layout shifts so that's kind of a big deal as far as web vitals are concerned.

What changed from #73 is that I'm reusing the same ajax object instead of creating a new one each time and I get rid of it at the end.

Removed the requestanimationframe, since that's not what it's for and it doesn't help too much with layout shifts and it delays the LCP a few ms compared to calling it directly.

I think this broke the cke5 admin ui though, will need to look into it.

yeah cke5 admin is messed up because the context for the behavior is not what's expected by the code. CKE expects document but bigpipe gives ul.toolbar-menu. The once call is missing the context argument. Fix looks big but it's just space/indent changes. Check the compiled file it's pretty clear :)

// From
once('ckeditor5-admin-toolbar', '#ckeditor5-toolbar-app').forEach()

// To
once('ckeditor5-admin-toolbar', '#ckeditor5-toolbar-app', context).forEach()

Related to #88 about list of ajax objects

#89: nice bug find 🤠

1. #88 interdiff:

   +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,18 +3,38 @@
   +  });
   +  /**
   

   Übernit: probably needs an empty line in between?

2. #88 interdiff:

   +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -3,18 +3,38 @@
   -   *   The text content of a <script type="application/vnd.drupal-ajax"> DOM node.
   +   *   The text content of a <script type="application/vnd.drupal-ajax"> DOM
   +   *   node.
   

   Why this reflowing into 80 chars? I thought the 80 char limit didn't apply to JS?

   (Trying to reduce the patch to the smallest size possible.)

3. #89 interdiff:
   Could you please not reflow all that (out-of-scope) CKE5 admin JS code? That makes this diff much harder to read 🙈

I let prettier do it's thing. If we don't reflow it'll fait commit checks

probably needs reroll following #3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed

reroll to 10.0.x branch, added update from #3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed.

#94.1 : fixed
#94.2 : I've always kept the 80 chars limit in js files too.
#94.3 : autoformat, can't do much about that. The diff to the built file is pretty clear about the changes that are introduced.

we'll need a different patch for 9.5 (need to add a polyfill at least)

#85 is very promising, #94 was addressed, the patch otherwise looks good to me and I have no review comments -> RTBC.

Setting this to Unassigned since this is now RTBC.

Random fail in 1) Drupal\Tests\book\Functional\BookTest::testGetTableOfContents? Re-testing, re-RTBC'ing.

updated patch after js file renames

I did some manual testing that was requested on #77 using an iPhone 5 which was the device with lowest CPU performance that I had access to and could debug reasonably. I didn't see any regressions in the experience but I think the phone might have had too high CPU performance to prove this isn't the case with any phone. I also couldn't get access to the CPU usage so this was all based on perceived performance and timelines.

I was able to confirm higher CPU usage, and higher peak CPU usage on my newer iPhone. On this device the change was very moderate but I think it's enough to justify some extra investigation with a lower performance device.

All right, dusted my old hardware. One was way to old to even run devtools (it's on android 4.4) The oldest I could reasonably get to work was a galaxy A5 (2016), it has a pretty poor geekbench score (less than half of the iphone 5) so I guess it's a good stand-in for cheap current hardware, want's that expensive at the time either.

For the test I tweaked the BigPipeTestController::multiOccurrence controller to add 100 placeholders and an additional item that renders with a delay of 1 second. Removed toolbar/contextual links that were adding noises to measures.

Here are both traces. In short it doesn't change much. The bottle neck is in the DOM/jQuery/attachbehaviors when adding things to the page. I tried changing the script to use a queue that is processed every 50ms instead of as soon as the node is added to the DOM, so we add like 80+ placeholders at once. Because we process each ajax command separately it doesn't change much, just increases the time spent in scripting. I also tried to group the batched commands into one call of the ajaxobject.success method. Same thing, each command need to run attachbehavior individually so it doesn't help.

All in all there might be a bit more cpu usage but it might just be because it uses the CPU more efficiently. It seems we gain a few ms overall when using mutation observer but it's not significant enough to be impacting user experience. This is good to go as is just for the easier to understand code.

That book test sure is annoying

Thanks for doing that deep testing!

Barely a CPU difference when rendering the rather extreme case of 100 placeholders … that sounds like the kind of validation @lauriii requested in #104 😊

Also: memory consumption is lower — from 2.4 MB down to 2.1 MB (~12% lower). Which means we can reasonably assume there are likely fewer cache evictions in the browser on lower end hardware with this change too. Nice win!

I think this is ready :)

This is a great improvement, using a Mutation Observer is a much more performant approach and it's good to see us using a more appropriate native browser API. Patch looks good - also tested it manually and it works as expected - +1

Committed 7bf0b38 and pushed to 10.1.x. Thanks!

This has resulted in a bug in Olivero - see #3319325: Olivero: Mobile menu does not work when authenticated and BigPipe enabled(D10 only). I discussed this with @nod_ and we agreed that ideally this issue should not result in a breaking change. @nod_ will file a followup issue to:

we could pass document in the bigpipe call to attachbehaviors to avoid that

There's #3160052: Initial argument passed to Drupal behaviors is incorrect