Unnecessary optimization in wp_get_ready_cron_jobs()

[lev0]

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

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

[SergeyBiryukov]

Hi there, thanks for the ticket!

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

@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 ) {

}}}

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 ) ) {

}}}

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.

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.

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".

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.

@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.

@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.

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.

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

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

[peterwilsoncc]

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

[SergeyBiryukov]

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:

• [3634] / #2425
• [4077] / #2715
• [8927] / #7068
• [10521] / #9005

[mukesh27]

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?

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.)

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.

[peterwilsoncc]

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.

Merged in https://core.trac.wordpress.org/changeset/54110 / 42d3773de9

[johnbillion]

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

[desrosj]

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.