Make WordPress Core

Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#59439 closed enhancement (fixed)

Blocks: Remove `inject_hooked_block_markup` filter

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: bernhard-reiter's profile Bernhard Reiter
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

Per discussion in #59424, there's agreement that the new hooked_block_types filter (introduced in [56673]) covers conditional addition and removal of hooked blocks better and at a higher level than the inject_hooked_block_markup filter that was originally added in [56649] for that same purpose, and that the latter should thus be removed.

Change History (9)

This ticket was mentioned in PR #5292 on WordPress/wordpress-develop by @Bernhard Reiter.


7 months ago
#1

  • Keywords has-patch added

Per discussion in #5271, there's agreement that the new hooked_block_types filter covers conditional addition and removal of hooked blocks better and at a higher level than the inject_hooked_block_markup filter that was originally added in #5261 for that same purpose, and that the latter should thus be removed.

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

@gziolo commented on PR #5292:


7 months ago
#2

@felixarntz, I'm wondering how much we can trust this performance job:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/699132/f6a3b6fb-0c79-4d07-9204-66a15b70ca2b

We don't have any hooked blocks in WordPress core, so there should be no difference between this branch in trunk. There is going to be improvement when using plugins, but we can't measure that with this job.

#3 @Bernhard Reiter
7 months ago

  • Keywords commit added

#4 @Bernhard Reiter
7 months ago

  • Owner set to Bernhard Reiter
  • Resolution set to fixed
  • Status changed from new to closed

In 56674:

Blocks: Remove inject_hooked_block_markup filter.

Per discussion in #59424, there's agreement that the new hooked_block_types filter (introduced in [56673]) covers conditional addition and removal of hooked blocks better and at a higher level than the inject_hooked_block_markup filter that was originally added in [56649] for that same purpose.
Consequently, this changeset removes the latter filter.

Props gziolo.
Fixes #59439.

@flixos90 commented on PR #5292:


7 months ago
#6

@gziolo It looks strange that this appears to have such an impact in classic themes indeed (see https://github.com/WordPress/wordpress-develop/actions/runs/6297076920). Though if this function is called _very_ often on page load, it could have an actual impact to just remove the filter appliance - though I can't imagine that is the case, as it would need to be _extremely_ often 😆

We are aware of sometimes notable variance in the performance test results, but generally Server-Timing has been much more stable than the Web Vitals, so this difference is confusing to see.

@swissspidy Do you have any idea?

@swissspidy commented on PR #5292:


7 months ago
#7

I'm sure y'all have seen the Due to the nature of how GitHub Actions work, some variance in the results is expected. disclaimer at the top. The comparison table is a nice addition, but we still have a lot of work to do to reduce the variance in the current performance test setup, i.e. clearing caches, actually logging out the user, doing more iterations, etc.

In the meantime, best to compare the numbers with other runs on PR or trunk to get a better feeling for the variance.

@flixos90 commented on PR #5292:


7 months ago
#8

I just ran a benchmark for this locally using TT1, and it gave me:

  • 52.13ms without the PR
  • 53.4ms with the PR

That difference is small enough to be safely discarded as variance as there is also absolutely no way this could be slower as it's only removing a filter execution.

Still, seeing this kind of huge difference in the CI workflow makes me doubt its reliability at the moment. While a centrally visible and reusable workflow is certainly the goal, there may unfortunately still be value in running benchmarks individually on a personal machine as it seems the environment in GitHub Actions is far less controlled than a regular machine 😞

@swissspidy commented on PR #5292:


7 months ago
#9

It's known that GitHub Actions runners don't provide the most consistent environment. Results can change even depending on the time of day (and thus load). But that's all solvable to some degree :-)

Note: See TracTickets for help on using tickets.