Blocks: Remove block visitor indirection in hooked blocks insertion

[Bernhard Reiter]

It might make sense to remove the extra indirection (i.e. the make_before_block_visitor and make_after_block_visitor factories) that we currently have around the visitor callbacks that we pass to traverse_and_serialize_blocks.

The original idea behind that indirection was a clean-room level separation of concerns: While I wanted the hooked_block_types filter to be aware of the template, template part, or pattern that it belongs to, I wanted to keep traverse_and_serialize_block(s) perfectly unaware of this extra information. The best way I could think of at the time was the indirection through the make_ factories.

However, I'm coming round to considering the alternative of passing the extra context to traverse_and_serialize_block(s) after all, and then have that function pass it as an argument to the callbacks. Importantly, to preserve separation of context, for the purpose of traverse_and_serialize_block(s), I'd like the extra data to be completely generic (i.e. a mixed datatype with no assumptions with regard to what it contains); it's solely up to the caller what that extra data looks like, and how it is used by the callbacks.

This might help improve performance of Block Hooks related logic a bit. Furthermore, it unlocks the ability to pass additional data to the callbacks.

As discussed with @gziolo, it might make sense to remove the extra indirection (i.e. the make_before_block_visitor and make_after_block_visitor factories) that we currently have around the visitor callbacks that we pass to traverse_and_serialize_blocks.

The original idea behind that indirection was a clean-room level separation of concerns: While I wanted the hooked_block_types filter to be aware of the template, template part, or pattern that it belongs to, I wanted to keep traverse_and_serialize_block(s) perfectly unaware of this extra information. The best way I could think of at the time was the indirection through the make_ factories.

However, I'm coming round to considering the alternative of passing the extra context to traverse_and_serialize_block(s) after all, and then have that function pass it as an argument to the callbacks. Importantly, to preserve separation of context, for the purpose of traverse_and_serialize_block(s), I'd like the extra data to be completely generic (i.e. a mixed datatype with no assumptions with regard to what it contains); it's solely up to the caller what that extra data looks like, and how it is used by the callbacks.

This might help improve performance of Block Hooks related logic a bit. Furthermore, it unlocks the ability to pass additional data to the callbacks. @gziolo E.g. for #5399, we might change $data to include both $context and $hooked_blocks :blush:

TODO:

• [ ] Add unit test coverage. Since the $data/$context arg is only used for the hooked_block_types filter, which isn't currently used by Core, it's important to make sure that the information passed to traverse_and_serialize_block(s) is successfully communicated to the callbacks.

Trac ticket: https://core.trac.wordpress.org/ticket/59549

I did benchmarking using the same method as described in ​https://github.com/WordPress/wordpress-develop/pull/5399#issuecomment-1748754479:

npm run research -- benchmark-server-timing -u http://localhost:8889/2023/10/05/benchmarking/ -n 100 -p

This branch:

╔════════════════════════════════╤════════════════════════════════════════════════╗
║ URL │ ​http://localhost:8889/2023/10/05/benchmarking/ ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Success Rate │ 100% ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p10) │ 308.54 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p25) │ 310.12 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p50) │ 313.23 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p75) │ 318.91 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p90) │ 336.07 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p10) │ 2.28 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p25) │ 2.32 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p50) │ 2.39 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p75) │ 2.46 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p90) │ 2.55 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p10) │ 123.77 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p25) │ 124.54 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p50) │ 125.57 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p75) │ 129.96 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p90) │ 149.69 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p10) │ 180.44 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p25) │ 180.87 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p50) │ 181.78 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p75) │ 185.08 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p90) │ 189.43 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p10) │ 305.08 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p25) │ 306.28 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p50) │ 309.24 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p75) │ 315.23 ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p90) │ 331.54 ║
╚════════════════════════════════╧════════════════════════════════════════════════╝

This branch with the ​Like Button plugin installed and activated.

╔════════════════════════════════╤════════════════════════════════════════════════╗
║ URL                            │ http://localhost:8889/2023/10/05/benchmarking/ ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Success Rate                   │ 100%                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p10)            │ 313.1                                          ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p25)            │ 314.16                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p50)            │ 315.85                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p75)            │ 322.33                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ Response Time (p90)            │ 335.61                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p10) │ 2.26                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p25) │ 2.29                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p50) │ 2.34                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p75) │ 2.41                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-load-alloptions-query (p90) │ 2.51                                           ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p10)       │ 123.74                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p25)       │ 124.96                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p50)       │ 125.62                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p75)       │ 127.84                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-before-template (p90)       │ 142.54                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p10)              │ 184.77                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p25)              │ 185.07                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p50)              │ 185.94                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p75)              │ 187.48                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-template (p90)              │ 193                                            ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p10)                 │ 309.41                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p25)                 │ 310.56                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p50)                 │ 312.36                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p75)                 │ 318.84                                         ║
╟────────────────────────────────┼────────────────────────────────────────────────╢
║ wp-total (p90)                 │ 332.3                                          ║
╚════════════════════════════════╧════════════════════════════════════════════════╝

[hellofromTonya]

The functions were introduced in [59313] during the 6.4 cycle / #59313.

​After discussing in Make/Core slack channel, the ticket exists to deal with performance issues of newly introduced functionality. Thus, it's more of a bugfix for performance than an enhancement.

Caution: During beta, there's a fine line between fixing performance and enhancing for refinement. Beta is for fixing issues.

As @joemcgill ​about converting into a bugfix ticket so that it can continue in 6.4:

No concerns from me, assuming it's really isolated to a performance optimization and isn't adding any new functionality.

@ockham @gziolo Apologies in advance, I am lacking context here completely on the functionality behind these changes, so what I did may be wildly inaccurate... But I just ran a few benchmarks and unfortunately couldn't replicate any notable benefits of this PR.

I used the benchmark-server-timing command, with 100 runs each, and I did that 3 times (i.e. 3 medians, each based on 100 runs). Below is a summary of the wp-total metrics my data.

TT3 home page:

• 63.09ms (PR) vs 63.64ms (trunk)
• 64.95ms (PR) vs 65.3ms (trunk)
• 63.87ms (PR) vs 63.4ms (trunk)

TT4 home page:

• 90.03ms (PR) vs 90.08ms (trunk)
• 92ms (PR) vs 91.13ms (trunk)
• 90.04ms (PR) vs 90.03ms (trunk)

TT3 Hello world post (with Like Button plugin active):

• 73.16ms (PR) vs 72.85ms (trunk)
• 74.18ms (PR) vs 73.62ms (trunk)
• 73.19ms (PR) vs 74.27ms (trunk)

Based on the above, there doesn't seem to be a notable difference, sometimes it's a tiny bit faster, sometimes a tiny bit slower, which is probably mostly just variance.

There's a good chance I am testing this in the wrong configuration. Can you provide a bit more context on how I need to set up the WP site? Do I need to configure the plugin somehow? FWIW after activating it, all I saw was a Like button under the "Hello world!" post comment, but nowhere else - not sure that's correct.

@felixarntz, thank you so much for running the tests on your end. It's the first time I did benchmarking for WordPress core, so there might issues in my setup or the approach I took. Let me run the same tests as you did on this branch and trunk so I can confirm I see similar results.

I would also appreciate it if you could validate my findings for another PR ​https://github.com/WordPress/wordpress-develop/pull/5399 that is trying different optimizations on how the get_hooked_blocks works and is used in the code, which seems promising, but again it might be the setup I had that is favorable for how the branch works.

I tried the same approach locally and I tested with TT3 Hello world post (with Like Button plugin active):

229.54 (PR) vs 227.19 (trunk)
226.99 (PR) vs 231.02 (trunk)
227.14 (PR) vs 226.68 (trunk)

This aligns with the findings that @felixarntz shared 🤔

By the way, I'm using npm run env:start with Docker. I assume the reason why the response time is multiplied by a factor of 3 on my machine.

Thanks @gziolo!

By the way, I'm using npm run env:start with Docker. I assume the reason why the response time is multiplied by a factor of 3 on my machine.

The time between these benchmarks usually differs wildly between different machines, so that's not an issue at all. We just have to ensure the environment in which the benchmarks that are compared with each other are run is as consistent as possible. So it's perfectly okay your numbers are _overall_ so different from mine.

That said, I'm also using the Docker core dev environment for these benchmarks, via npm run env:start, so that alone isn't it. Are you maybe having XDebug enabled? Or WP_DEBUG as true? I usually disable any of those things when benchmarking as they can slow things down (especially profiling like XDebug). Again, it shouldn't matter too much though, as long as your own benchmarks use the same consistent environment.

That said, I'm also using the Docker core dev environment for these benchmarks, via npm run env:start, so that alone isn't it. Are you maybe having XDebug enabled? Or WP_DEBUG as true? I usually disable any of those things when benchmarking as they can slow things down (especially profiling like XDebug). Again, it shouldn't matter too much though, as long as your own benchmarks use the same consistent environment.

This is part of my wp-config.php:

I don't think I have XDebug enabled.

Thanks a lot for running the benchmark timings on this, @felixarntz and @gziolo!

Since it looks like the removal of the indirection doesn't have any noticeable effect, I'm going to close this PR (and ticket). Looks like #5399 is much more promising 😄

[Bernhard Reiter]

Per the benchmark timings shared on the PR, there doesn't seem to be any noticeable effect to removing the indirection. I'm thus also closing this ticket.