Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#56541 closed enhancement (fixed)

Performance: Don't execute jumps in loops

Reported by: cybr's profile Cybr Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch commit
Focuses: performance Cc:

Description

To spare CPU time stalling on branch predictors (briefly explained), it's better to execute as few jumps in the code, especially when we can easily modify them. The simplest form of implementing a jump in PHP is via an if-statement. Executing those inside loops will increase CPU time quickly.

For example, and probably the most significant, in wpdb::get_results(), the following code block could be converted to the next code block.

Current:

<?php
foreach ( (array) $this->last_result as $row ) {
        if ( ARRAY_N === $output ) {
                // ...integer-keyed row arrays.
                $new_array[] = array_values( get_object_vars( $row ) );
        } else {
                // ...column name-keyed row arrays.
                $new_array[] = get_object_vars( $row );
        }
}

My proposal:

<?php
if ( ARRAY_N === $output ) {
        foreach ( (array) $this->last_result as $row ) {
                // ...integer-keyed row arrays.
                $new_array[] = array_values( get_object_vars( $row ) );
        }
} else {
        foreach ( (array) $this->last_result as $row ) {
                // ...column name-keyed row arrays.
                $new_array[] = get_object_vars( $row );
        }
}

Yes, that is more code outputting the same data, but it executes faster. To get an indication of the time saved when querying 2000 items, see https://3v4l.org/KvpIR#tabs.

Across the board, there's about a 10% performance improvement for the if branch, and 20% for the else branch. When we loop over 80,000 results, which I don't think is out of the ordinary, the results become significant: https://3v4l.org/6OOHn#tabs. This is by optimizing just one foreach loop. Applying these savings everywhere could yield not only measurable but noticeable improvements.

I found another example at WP_Upgrader_Skin::error(), withal which keeps calling the same WP_Error::get_error_data() method up to three times per loop:

<?php
foreach ( $errors->get_error_messages() as $message ) {
        if ( $errors->get_error_data() && is_string( $errors->get_error_data() ) ) {
                $this->feedback( $message . ' ' . esc_html( strip_tags( $errors->get_error_data() ) ) );
        } else {
                $this->feedback( $message );
        }
}

Therefor my proposal, to which I added a trim() call to spare us an if-statement all the same:

<?php
$error_data = $errors->get_error_data();
$error_data = is_string( $error_data ) ? esc_html( strip_tags( $error_data ) ) : '';

foreach ( $errors->get_error_messages() as $message ) {
        $this->feedback( trim( "$message $error_data" ) );
}

The principle is this: Do not execute work inside a loop that should be done outside of it.

Change History (11)

#1 @Cybr
2 years ago

  • Summary changed from Performance: Don't execute jumps in arrays to Performance: Don't execute jumps in loops

#2 @johnbillion
2 years ago

  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

@Cybr 👍 Do you use an automated tool to identify these or is it manual?

#3 @Cybr
2 years ago

It caught my eye as I needed to study the code.

I found the second example as I tried to create a crude lookup "tool" for it using regular expressions, but it doesn't work well in JS, plus it yields many false positives in VSCode: https://regex101.com/r/nkZIUZ/2.

#4 @costdev
2 years ago

The results speak for themselves IMO. Nice work @Cybr!

The principle is this: Do not execute work inside a loop that should be done outside of it.

Should this be considered for an entry in the handbook, I would revise this to:

Do not execute work inside a loop that can be done outside of it.

That way, personal opinion regarding "should" doesn't come into it.

#5 @Collizo4sky
2 years ago

Good work @Cybr

#6 @spacedmonkey
20 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to spacedmonkey
  • Status changed from new to reviewing

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


20 months ago
#7

  • Keywords has-patch added; needs-patch removed

#8 @spacedmonkey
20 months ago

  • Milestone changed from 6.4 to 6.3

#9 @spacedmonkey
20 months ago

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

In 56066:

Database: Move the if statement outside of the loop.

In the foreach loop of last results, move the if statement outside of the loop. There is no need to check every element in the array for the output type. Do this once outside of the loop. For large database queries with lots of rows returned, this should improve PHP performance.

Props spacedmonkey, Cybr, johnbillion, costdev, joemcgill.
Fixes #56541.

#10 @spacedmonkey
20 months ago

  • Keywords commit added

@Cybr I committed the database tweak you noted. If you have any other examples, like the get_error_data, let's do these as other smaller tickets in WP 6.4.

@spacedmonkey commented on PR #4719:


20 months ago
#11

Committed.

Note: See TracTickets for help on using tickets.