Make WordPress Core

Opened 14 months ago

Closed 13 months ago

Last modified 13 months ago

#59405 closed enhancement (fixed)

Reduce the use of the `_wp_array_get()` function to improve performance

Reported by: aristath's profile aristath Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch gutenberg-merge
Focuses: performance Cc:

Description (last modified by hellofromTonya)

Merges work from Gutenberg to reduce significantly reduce the number of calls to _wp_array_get(), to improve performance.

This ticket merges the same changes from Gutenberg into Core, and applies the same logic where appropriate.

References:

Change History (19)

#1 @aristath
14 months ago

  • Focuses performance added

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


14 months ago
#2

  • Keywords has-patch added

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


14 months ago

@mukesh27 commented on PR #5244:


14 months ago
#4

@aristath thanks for the PR. Could you please share the performance number before after the changes?

@aristath commented on PR #5244:


14 months ago
#5

@aristath thanks for the PR. Could you please share the performance number before after the changes?

Trunk:
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/588688/3d147916-897e-4700-96fa-2cc3cf19f115

After this PR:
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/588688/ffb4f9ef-33a1-4102-82d2-4bc0aa1db535

So on my test, we're saving 2582 call to the _wp_array_get() function, and 39ms.
These results are not accurate since XDebug does add a lot of overhead, but there is a clear improvement.

@swissspidy commented on PR #5244:


14 months ago
#6

The coding standards disallow using the null coalescing operator right now, see https://core.trac.wordpress.org/ticket/58874

@jrf commented on PR #5244:


14 months ago
#7

The coding standards disallow using the null coalescing operator right now, see https://core.trac.wordpress.org/ticket/58874

The coding standards do no such thing: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/

The coding standards have no opinion on whether you use it or not. It's a matter of the minimum PHP version.

As for that Trac ticket: that's about whether to do a bulk update of the Core code base for it (and possibly enforce its use), which is a different matter.

@aristath commented on PR #5244:


14 months ago
#8

Thank you @swissspidy 👍
Technically it's not "disallowed", it's just not used _yet_. If it was disallowed, tests would be failing.

Do you think it would be better if I change these to be isset() checks instead in this PR?
Do you believe that would that improve the readability of the code?

@swissspidy commented on PR #5244:


14 months ago
#9

My bad, just glanced over it on mobike. Thanks for correcting me!

@spacedmonkey commented on PR #5244:


14 months ago
#10

As this feels like a code qualiaty thing, I am going to ping @SergeyBiryukov

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


13 months ago

@aristath commented on PR #5244:


13 months ago
#12

Rebased the PR, resolving conflicts 👍

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


13 months ago

#14 @hellofromTonya
13 months ago

  • Description modified (diff)
  • Keywords gutenberg-merge added
  • Summary changed from Backport from Gutenberg: Reduce the use of the `_wp_array_get()` function to improve performance to Reduce the use of the `_wp_array_get()` function to improve performance

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


13 months ago

@aristath commented on PR #5244:


13 months ago
#16

@hellofromtonya if the use of ?? is considered a blocker in this PR, I can simply change it to isset() checks.
The performance improvements from this PR are the same regardless of which syntax we use, and the null coalescing operator is not the main point of this PR.

Would that help us move this ticket forward? We can convert isset() checks to ?? at a later date if we want.

@aristath commented on PR #5244:


13 months ago
#17

Thank you all for the feedback!

I pushed a commit, changing the syntax so it doesn't use ?? anymore, and it now uses isset() checks. 👍

#18 @SergeyBiryukov
13 months ago

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

In 56709:

Editor: Reduce the use of the _wp_array_get() function to improve performance.

_wp_array_get() is an expensive function, and it's called thousands of times on each page view on the front end. While the function performance was slightly improved in #58376, it is still called more times than it should be.

This commit aims to further optimize its usage:

  • In many cases, _wp_array_get() can be replaced with a much simpler and faster isset() check.
  • The isset() function is capable of checking nested arrays, so isset( $foo['a']['b']['c'] ) will return false even if $foo['a'] is unset, without throwing any errors or warnings.
  • When _wp_array_get() cannot be directly replaced with isset(), it would be good practice to wrap it in an isset() function so that _wp_array_get() only runs when it needs to.

Original PR from Gutenberg repository:

Follow-up to [55851], [56382].

Props aristath, jrf, spacedmonkey, mukesh27, swissspidy, hellofromTonya.
Fixes #59405.

@SergeyBiryukov commented on PR #5244:


13 months ago
#19

Thanks for the PR! Merged in r56709.

Note: See TracTickets for help on using tickets.