Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 21 months ago

#56092 closed defect (bug) (fixed)

Unnecessary optimization in wp_get_ready_cron_jobs()

Reported by: lev0's profile lev0 Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: trivial Version: 5.1
Component: Cron API Keywords: has-patch commit
Focuses: performance Cc:

Description

The short circuit just before iterating the $crons has no performance benefit. See https://github.com/WordPress/wordpress-develop/commit/583d254d4f5538dc7d0edd779fc21a302694b8de

Change History (25)

This ticket was mentioned in PR #2919 on WordPress/wordpress-develop by Roy-Orbison.


2 years ago
#1

  • Keywords has-patch added

The foreach loop would break on the first iteration, so all it does is make the function a tiny bit slower when there *are* jobs.

This PR just removes the extra steps.

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

#2 @dingo_d
2 years ago

  • Focuses performance added; coding-standards removed

#3 follow-up: @SergeyBiryukov
2 years ago

  • Version changed from trunk to 5.1

Hi there, thanks for the ticket!

Introduced in [44483] / #45797, setting the version accordingly.

#4 @davidbaumwald
2 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to davidbaumwald
  • Status changed from new to reviewing

Roy-Orbison commented on PR #2919:


2 years ago
#5

@jrfnl That seems like a good idea as it would avoid the system call from microtime. I prefer testing truthiness of arrays, myself, as I think checking whether it contains anything is a simpler operation in PHP's internals, i.e.:
{{{php

if ( ! is_array( $crons )
! $crons ) {

}}}

jrfnl commented on PR #2919:


2 years ago
#6

I prefer testing truthiness of arrays, myself, as I think checking whether it contains anything is a simpler operation in PHP's internals, i.e.:

Well, each to their own I suppose, but to me, that is a "Yikes" as it raises the cognitive complexity unnecessarily.

I haven't studied the internals of this for PHP, but it really shouldn't make much difference - we're talking micro-optimization of a micro-optimization -, but if you really prefer not to use the strict comparison, I'd suggest to use empty() instead as that is highly optimized, what with it being a language construct, and using that doesn't raise the cognitive complexity as it will be clear why that check is being done.

{{{php

if ( ! is_array( $crons )
empty( $crons ) ) {

}}}

Roy-Orbison commented on PR #2919:


2 years ago
#7

I think ! I antsy most concise and obvious. Also, empty() is only to guards against falsey variables that may bit exist, so shouldn't be used if you know it's already been initialised.

jrfnl commented on PR #2919:


2 years ago
#8

I think ! is the most concise and obvious.

There is nothing obvious about ! $var as it doesn't tell you what you are actually checking for. It's the bane of WordPress and IMO should be banned from the code-base completely in favour of explicit conditions, which self-document what the code is supposed to check for (or against).

Also, empty() is only to guard against falsey variables that may not exist, so shouldn't be used if you know it's already been initialised.

Sorry, but that really is nonsense.
empty() _includes protection_ against uninitialized variables, but saying that it should only be used when you don't know whether the variable exists, is turning the world upside-down. The purpose of empty() is to check whether a variables is "empty" - like in this case an empty array - , so let's use it for the exact purpose the language construct exists for.

Roy-Orbison commented on PR #2919:


2 years ago
#9

It's really not nonsense, empty($x) is exactly equivalent to !isset($x) || !$x. When I see use empty when the variable is declared right above it, it reads like a lack of knowledge about PHP. It also serves to emit warnings during development when a variable _should_ exist, but a mistake has been made that will be suppressed by the empty call. Using it implies the developer should expect the variable may not exist.

Commit whatever you want, or don't, but don't tell me ! is "yikes" and "nonsense".

jrfnl commented on PR #2919:


2 years ago
#10

It also serves to emit warnings during development when a variable should exist, but a mistake has been made

Which is exactly why the use of ! $var should be banned in favour of explicit conditions. _Those type of mistakes shouldn't be allowed to enter the codebase_.

! $var is a frequent cause of bugs as it is used as a shortcut instead of doing proper type and content checks.

Few people use it correctly (you may be one of them), most don't and it teaches the wrong kind of coding to novices. Keep in mind, contributing to the WordPress codebase should not be reserved to senior devs, but should also be accessible to less experienced people.

peterwilsoncc commented on PR #2919:


2 years ago
#11

@Roy-Orbison Thank you for your patch.

I'm currently working towards committing https://core.trac.wordpress.org/ticket/53940 to change _get_cron_array() to return a consistent type, see PR #3038.

As this removes the need to check the return type in wp_get_ready_cron_jobs(), committing it will cause a merge conflict in this pull request.

Honestly, I am not overly concerned whether the check is falsey or empty(). My inclination is to skip it alongside the other code you suggest and in the rare event the cron array is empty let the foreach run on an empty array.

WordPress schedules a number of events by default so the likelihood _get_cron_array() will return [] is very slim.

Roy-Orbison commented on PR #2919:


2 years ago
#12

@peterwilsoncc In that case, the first changed lines could be modified as below, and the latter deletions in this PR would still prevent twice-checking the array contents.

{{{php
if ( empty( $crons ) ) {

return $crons;

}}}
I'm happy to update this PR after Hardik Thakkar's patch is merged.

peterwilsoncc commented on PR #2919:


2 years ago
#13

I'm happy to update this PR after Hardik Thakkar's patch is merged.

Thank you, kindly.

I'll let you know when I've committed it -- I hope to do so today but am just making sure the existing tests pass and adding in some coverage.

peterwilsoncc commented on PR #2919:


2 years ago
#14

I've committed the other ticket and, as promised, this has introduced merge conflicts to this PR.

Roy-Orbison commented on PR #2919:


2 years ago
#15

Sorry for the commit + CI noise, I rebased to ease merging but was still in the detached HEAD mode. Is the patch okay now?

#16 @peterwilsoncc
23 months ago

  • Keywords commit added

The linked pull request looks good for commit as of 050fd09c

#17 in reply to: ↑ 3 @SergeyBiryukov
23 months ago

Replying to SergeyBiryukov:

Introduced in [44483] / #45797, setting the version accordingly.

Looking a bit closer, while the function was introduced in [44483], the actual short-circuit fragment is older than that.

Some more history here:

#18 @mukesh27
22 months ago

We discussed this in today's bug scrub in the Performance channel. @SergeyBiryukov The PR got some approval. Do you have any concerns to discuss? Or is it ready for the merge?

Roy-Orbison commented on PR #2919:


22 months ago
#19

My inclination is to skip it alongside the other code you suggest and in the rare event the cron array is empty let the foreach run on an empty array.

I should have better heeded that. If the cron array is virtually never empty in practice, the if is another unnecessary optimisation.

I can just simplify the whole patch to this:
{{{patch
--- a/src/wp-includes/cron.php
+++ b/src/wp-includes/cron.php
@@ -1122,23 +1122,20 @@ function wp_get_ready_cron_jobs() {

  • to continue using results from _get_cron_array(). */

$pre = apply_filters( 'pre_get_ready_cron_jobs', null );

+

if ( null !== $pre ) {

return $pre;

}


$crons = _get_cron_array();

-

$gmt_time = microtime( true );

  • $keys = array_keys( $crons );
  • if ( isset( $keys[0] ) && $keys[0] > $gmt_time ) {
  • return array();
  • }

-

$results = array();

+

foreach ( $crons as $timestamp => $cronhooks ) {

if ( $timestamp > $gmt_time ) {

break;

}

+

$results[ $timestamp ] = $cronhooks;

}

}}}

(It includes Sergey's added space.)

peterwilsoncc commented on PR #2919:


22 months ago
#20

I can just simplify the whole patch to this: [snip]

That works for me.

It's still unmerged so feel free to update the PR if you wish.

#21 @peterwilsoncc
22 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54110:

Cron API: Remove unnecessary optimization getting ready events.

Remove the check for future events prior iterating the array of crons in wp_get_ready_cron_jobs(). If there are no ready events, the foreach loop will break on the first iteration so the optimization is not required.

As WordPress Core adds a number of events by default, accounting for an empty array is not required in most instances.

Props lev0, jrf, SergeyBiryukov.
Fixes #56092.

#23 @milana_cap
22 months ago

  • Keywords add-to-field-guide added

#24 @johnbillion
22 months ago

  • Keywords add-to-field-guide removed

This doesn't need a field guide entry, it's a minor internal optimisation

#25 @desrosj
21 months ago

In 54210:

Coding Standards: Various alignment fixes from composer format.

Follow up to [53874], [54097], [54110], [54155], [54162], [54184].

See #39210, #55443, #56288, #56092, #56408, #56467, #55881.

Note: See TracTickets for help on using tickets.