Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#59181 closed defect (bug) (fixed)

Performance optimization of register_block_script_handle()

Reported by: mukesh27's profile mukesh27 Owned by: antpb's profile antpb
Milestone: 6.4 Priority: normal
Severity: normal Version: 5.5
Component: Editor Keywords: has-screenshots good-first-bug has-patch commit
Focuses: performance Cc:

Description

During profiling for TT3 home page, a performance bottleneck has been identified within the register_block_type_from_metadata function of the WordPress core. This function is responsible for a significant portion (approximately 8-10%) of the overall load time. Subsequent analysis reveals that the register_block_script_handle function, called within register_block_type_from_metadata, contributes to about 3% of the load time. Given these findings, there's a clear opportunity to enhance the performance of these functions.

Within the register_block_type_from_metadata function, the register_block_script_handle function is being invoked four times. This is a significant contributor to the overall load time. Further examination of the code shows that the plugins_url function is also called within this context. However, it appears that this call to "plugins_url" is relevant only for theme blocks.

Check attached screenshots for more details.

Attachments (2)

Before.png (444.6 KB) - added by mukesh27 11 months ago.
After.png (445.5 KB) - added by mukesh27 11 months ago.

Download all attachments as: .zip

Change History (19)

@mukesh27
11 months ago

@mukesh27
11 months ago

#1 @mukesh27
11 months ago

  • Focuses performance added

This ticket was mentioned in PR #5067 on WordPress/wordpress-develop by @daxelrod.


11 months ago
#2

  • Keywords has-patch added; needs-patch removed

Optimize register_block_script_handle() to only call plugins_url() if core or theme block conditions are not met.

#3 @antpb
11 months ago

  • Keywords reporter-feedback added

@mukesh27 Hello! I'm curious how you ran those performance tests. I'd love to try this on my own.

Is there any chance you can run this test again with the new PR that adds a fallback for the script uri? I'm curious if this has the performance gains we're hoping for.

#4 @adamsilverstein
11 months ago

  • Keywords commit added

#5 @antpb
11 months ago

  • Owner set to antpb
  • Resolution set to fixed
  • Status changed from new to closed

In 56455:

Performance: Add fallback for $script_uri to prevent firing plugins_url() unnecessarily.

Previously, in register_block_script_handle() the $script_uri variable initialized with a plugins_url() call that was reported to invoke four times. In this patch the var is initialized as a blank string with a fallback to use the plugins_url() if the other intended conditions are not met.

Props mukesh27, daxelrod, adamsilverstein, davidbaumwald.
Fixes #59181.

#6 @mukesh27
11 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 6.4

@antpb Moving this ticket to 6.4 milestone.

@oandregal commented on PR #5067:


11 months ago
#8

@mukeshpanchal27 @dougaxe1 would you be able to provide a before/after benchmarking? I mean, not a before/after profiling, but real numbers simulating a production environment – otherwise, we don't know the real impact.

If I understand how CI performance tests are setup for this (am I reading this right @felixarntz @joemcgill ?), this PR was committed as of https://github.com/WordPress/wordpress-develop/commit/0c82e15d72ee5114e61c676d6559d0763939d572 and so the performance tests action for it should be this one. According to those numbers, this has had a tremendous impact:

  • TTFB for classic theme has gone from 66.15ms to 59.69ms: -6.46ms, 9.76% faster.
  • TTFB for block themes has gone from 88.14ms to 62ms: -26ms, 29.65% faster.
Before After
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/078eb545-a6a7-4a5c-bda2-52dfa42d1f9b https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/07765190-c26e-4a26-8633-6c6b9aa6eebd

@daxelrod commented on PR #5067:


11 months ago
#9

Hey @oandregal, I too thought those numbers were too good to be true. Luckily I was sitting next to @davidbumwald who explained that TTFB numbers fluctuate pretty wildly in these automated tests, and maybe aren't quite representative of actual impact. An XHProf benchmark would be more accurate for these micro-optimizations.

@mukeshpanchal27 did I get that right?

@oandregal commented on PR #5067:


11 months ago
#10

If I understand how CI performance tests are setup for this (am I reading this right @felixarntz @joemcgill ?)

Ah, I think the response here is that I misunderstood the "base results". According to the workflow it seems that it compares the actual commit with the "base", which is 6.1.1. We've gone a long way since then, so the difference makes sense now.

@oandregal commented on PR #5067:


11 months ago
#11

Hey @oandregal, I too thought those numbers were too good to be true. Luckily I was sitting next to @davidbumwald who explained that TTFB numbers fluctuate pretty wildly in these automated tests, and maybe aren't quite representative of actual impact.

My experience is actually the contrary! TTFB results don't fluctuate much: around 2ms, perhaps. I think what happened here was that I misunderstood the actual numbers.

An XHProf benchmark would be more accurate for these micro-optimizations.

In the past, we claimed performance improvements that weren't such by basing the analysis on profilers. I hope we have learned since! So, my view is that every performance PR should attach a before/after benchmark based on production data.

For small things that don't take time, it's fine to provide it after the fact based on CI results, for example. For larger changes, it'd be useful if they are provided during the development, so authors/reviewers can gauge whether it's worth pursuing something or not before investing a lot of time investigating.

@oandregal commented on PR #5067:


11 months ago
#12

Actually, I just found out about https://github.com/WordPress/wordpress-develop/pull/5000 which is all what I've been wishing for! :)

@daxelrod commented on PR #5067:


11 months ago
#13

Thank you for the follow-up. I clearly misunderstood the performance tests too. Thank you for explaining, and yes, hoping for traction on #5000!

@mukesh27 commented on PR #5067:


11 months ago
#14

Thanks both of you!

@mukesh27 commented on PR #5067:


11 months ago
#15

@oandregal @dougaxe1 (cc. @joemcgill @felixarntz) I took a benchmarking for before after the change and it clear shows that it will improve Server side metric by ~1%. Check Sheet.

| Before | After | Diff % | Diff abs. |

-- | -- | -- | -- | -- |
wp-before-template (p10) | 174.37 | 173.14 | -0.71% | -1.23 |
wp-before-template (p25) | 176.22 | 175.01 | -0.69% | -1.21 |
wp-before-template (p50) | 179.74 | 177.84 | -1.06% | -1.90 |
wp-before-template (p75) | 187.68 | 180.18 | -4.00% | -7.50 |
wp-before-template (p90) | 197.96 | 182.97 | -7.57% | -14.99 |
wp-template (p10) | 194.4 | 192.85 | -0.80% | -1.55 |
wp-template (p25) | 195.73 | 194.32 | -0.72% | -1.41 |
wp-template (p50) | 198.42 | 195.89 | -1.28% | -2.53 |
wp-template (p75) | 202.58 | 198.11 | -2.21% | -4.47 |
wp-template (p90) | 209.61 | 200.27 | -4.46% | -9.34 |
wp-total (p10) | 370.29 | 368.03 | -0.61% | -2.26 |
wp-total (p25) | 372.5 | 370.43 | -0.56% | -2.07 |
wp-total (p50) | 377.61 | 373.77 | -1.02% | -3.84 |
wp-total (p75) | 389.8 | 377.52 | -3.15% | -12.28 |
wp-total (p90) | 402.07 | 379.7 | -5.56% | -22.37 |

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


11 months ago

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


11 months ago

Note: See TracTickets for help on using tickets.