Block Hooks: Remove block visitor indirection #5413
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It looks like we should explore it further as it seems to be a very promising idea: 
|
|
I did benchmarking using the same method as described in #5399 (comment): This branch:
This branch with the Like Button plugin installed and activated.
This branch with 5 hooked block types active on a page:
|
|
@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 TT3 home page:
TT4 home page:
TT3 Hello world post (with Like Button plugin active):
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 I would also appreciate it if you could validate my findings for another PR #5399 that is trying different optimizations on how the |
|
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 ( The same tests with TT4 Hello world post, the Like Button plugin active, and 4 additional blocks registered defining block hooks as above: 263.99ms (PR) vs 265.13ms (trunk) This aligns with the findings that @felixarntz shared 🤔 By the way, I'm using |
|
Thanks @gziolo!
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 |
This is part of my 
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 😄 |
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed with @gziolo, it might make sense to remove the extra indirection (i.e. the
make_before_block_visitorandmake_after_block_visitorfactories) that we currently have around the visitor callbacks that we pass totraverse_and_serialize_blocks.The original idea behind that indirection was a clean-room level separation of concerns: While I wanted the
hooked_block_typesfilter to be aware of the template, template part, or pattern that it belongs to, I wanted to keeptraverse_and_serialize_block(s)perfectly unaware of this extra information. The best way I could think of at the time was the indirection through themake_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 oftraverse_and_serialize_block(s), I'd like the extra data to be completely generic (i.e. amixeddatatype 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
$datato include both$contextand$hooked_blocks😊Trac ticket: https://core.trac.wordpress.org/ticket/59549
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.