WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#31182 closed defect (bug) (fixed)

array_shift in taxonomy.php very slow on large array

Reported by: VolodymyrC Owned by: SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.7
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

If wordpress blog have a large number posts (100000 or more), then this error has effect:

PHP Fatal error:  Maximum execution time of 30 seconds exceeded in /wpdir/wp-includes/taxonomy.php on line 1961

This is a code from line 1961 in taxonomy.php:

if ( 'id=>parent' == $_fields ) {
	while ( $term = array_shift( $terms ) ) {
		$_terms[$term->term_id] = $term->parent;
	}
} elseif ( 'ids' == $_fields ) {
	while ( $term = array_shift( $terms ) ) {
		$_terms[] = $term->term_id;
	}
} elseif ( 'names' == $_fields ) {
	while ( $term = array_shift( $terms ) ) {
		$_terms[] = $term->name;
	}
} elseif ( 'id=>name' == $_fields ) {
	while ( $term = array_shift( $terms ) ) {
		$_terms[$term->term_id] = $term->name;
	}
} elseif ( 'id=>slug' == $_fields ) {
	while ( $term = array_shift( $terms ) ) {
		$_terms[$term->term_id] = $term->slug;
	}
}

But array_shift function is very slow on large arrays.
http://kb.ucla.edu/articles/performance-of-array_shift-and-array_pop-in-php

Result:

array_pop takes 0.00089 seconds
array_shift takes 15.15544 seconds
array_reverse + array_pop takes 0.03934 seconds

My suggestion is make a change from array_shift to array_pop.

Attachments (1)

31182.diff (1.1 KB) - added by bswatson 6 years ago.

Download all attachments as: .zip

Change History (9)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Taxonomy
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.2
  • Version changed from 4.1 to 2.7

Originally introduced in [9652], changed in [12729] and [25161].

#2 follow-up: @jdgrimes
6 years ago

Just to confirm that array_pop() is faster:

$ php test.php
array_pop takes 0.00221 seconds
array_shift takes 5.69614 seconds
array_reverse + array_pop takes 0.03113 seconds
$ php -v
PHP 5.5.10 (cli) (built: Mar 18 2014 18:10:23) 
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
    with Xdebug v2.2.3, Copyright (c) 2002-2013, by Derick Rethans

I've run into issues with this as well with large taxonomies.

There are quite a few other places in core using array_shift(). Not all of these are subject to huge arrays, but I think they should probably be reviewed.

#3 @boonebgorges
6 years ago

array_shift() recalculates array indexes, which is why it's so slow on large arrays. See the first couple of comments here: http://php.net/manual/en/function.array-shift.php#86783 array_pop() would be fine. We could also use a foreach loop or even wp_list_pluck(), which became available only in 3.1.

#4 follow-up: @toscho
6 years ago

I don’t see why we should change the original array at all. We just have to move the pointer, right?

do {
    $term = current ( $terms );
    // add term to new array depending on condition
} while ( next( $terms );

#5 in reply to: ↑ 4 @boonebgorges
6 years ago

Replying to toscho:

I don’t see why we should change the original array at all. We just have to move the pointer, right?

do {
    $term = current ( $terms );
    // add term to new array depending on condition
} while ( next( $terms );

Right, or foreach. array_pop() seems unnecessary.

@bswatson
6 years ago

#6 @bswatson
6 years ago

  • Keywords has-patch added; needs-patch removed

Diff attached that changes

while ( $term = array_shift( $terms ) ) {

to

foreach ( $terms as $term ) {

Looping through via a do-while just seemed more complex and didn't add any benefit since the array is overwritten a few lines later. I also removed the reset just before the block as it's no longer needed if we aren't depending on the array pointer.

#7 in reply to: ↑ 2 @SergeyBiryukov
6 years ago

Replying to jdgrimes:

There are quite a few other places in core using array_shift(). Not all of these are subject to huge arrays, but I think they should probably be reviewed.

Created #31259 and #31260 for other instances in core and bundled themes.

#8 @SergeyBiryukov
6 years ago

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

In 31365:

Remove unnecessary array_shift() usage in get_terms() for better performance.

props bswatson, VolodymyrC.
fixes #31182.

Note: See TracTickets for help on using tickets.