#59405 closed enhancement (fixed)
Reduce the use of the `_wp_array_get()` function to improve performance
Reported by: | aristath | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch gutenberg-merge |
Focuses: | performance | Cc: |
Description (last modified by )
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)
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?
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
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
@
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
@
13 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 56709:
@SergeyBiryukov commented on PR #5244:
13 months ago
#19
Thanks for the PR! Merged in r56709.
This is a backport of https://github.com/WordPress/gutenberg/pull/51116 from Gutenberg
Trac ticket: https://core.trac.wordpress.org/ticket/59405