WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#31259 closed defect (bug) (fixed)

Replace array_shift() with current() where appropriate

Reported by: SergeyBiryukov Owned by: wonderboymusic
Milestone: 4.2 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing
Focuses: performance Cc:
PR Number:

Description

Background: #31182

array_shift() recalculates array indexes, and can be slow on large arrays. Most of the time, this is unnecessary, as we only use it to get the first element of the array, and the array is never used later.

Attachments (1)

31259.patch (8.2 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (8)

#2 @wonderboymusic
5 years ago

Yes, array_shift() does a lot:
https://github.com/php/php-src/blob/745504ea2a7066588af9714f5e32523a3fff4db7/ext/standard/array.c#L2199

I tend to prefer reset() over current(), because reset() is guaranteed to be the first item, while current() is based on the pointer at calltime. Internally, they are basically the same.

reset()
https://github.com/php/php-src/blob/745504ea2a7066588af9714f5e32523a3fff4db7/ext/standard/array.c#L889

current()
https://github.com/php/php-src/blob/745504ea2a7066588af9714f5e32523a3fff4db7/ext/standard/array.c#L922

Last edited 5 years ago by wonderboymusic (previous) (diff)

#3 @nacin
5 years ago

Strongly agree on not depending on the pointer being the first item.

This is definitely not the first time we've converted array modification functions to reset() and such. They sneak in over time.

#4 @wonderboymusic
5 years ago

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

In 31829:

Replace array_shift() with reset() where appropriate for performance.

Props SergeyBiryukov.
Fixes #31259.

#5 @SergeyBiryukov
5 years ago

In 31841:

After [31730], replace one more instance of array_shift() with reset() for better performance.

see #31259, #15459.

#6 @SergeyBiryukov
5 years ago

In 31842:

Bundled themes: After [31453], replace current() with reset(), which is guaranteed to be the first item.

see #31259, #31260.

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.