Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#50784 closed defect (bug) (fixed)

Stop using `preg_match()` in `wp_opcache_invalidate()`

Reported by: kirasong Owned by: kirasong
Priority: normal Milestone: 5.5
Component: Upgrade/Install Version: 5.5
Severity: normal Keywords: has-patch
Cc: Focuses: performance

Description

While writing up the devnote for wp_opcache_invalidate(), @aaroncampbell pointed out, rightly, that it might make sense to use something else to check for a PHP extension rather preg_match() for performance reasons.

As timeout failures are common during upgrades and we're only checking for one extension for PHP files at this point, I agree.

Patches coming shortly.

See: #36455

Attachments (3)

50784.substr.diff​ (469 bytes ) - added by kirasong 6 years ago.
50784.substr_compare.diff​ (475 bytes ) - added by kirasong 6 years ago.
50784.substr_compare_warning.diff​ (506 bytes ) - added by kirasong 6 years ago.

Download all attachments as: .zip

Change History (12)

#1 @kirasong
6 years ago

All of these are faster than what is there now.

50784.substr_compare.diff​ is the fastest here, but I noticed that in the ​PHP docs, a warning gets thrown if the string is too short, so 50784.substr_compare_warning.diff​ works around that.

Since that comes out pretty terse, I like the approach @jnylen0 used in a previous patch, in 50784.substr.diff​ quite a bit. This is slower, but much more readable.

Would appreciate any opinions.

Version 0, edited 6 years ago by kirasong (next)

#2 @SergeyBiryukov
6 years ago

50784.substr.diff​ would be my preference here, that seems the most consistent with other similar checks in core.

This ticket was mentioned in ​Slack in #core by david.baumwald. ​View the logs.


6 years ago

#4 @SergeyBiryukov
6 years ago

  • Keywords commit added

#5 @desrosj
6 years ago

Of the three approaches, I think I too prefer 50784.substr.diff​.

At first, another option that I thought of was using strpos() or stripos(). In my testing, this was the most performant approach. However, negative offsets are only supported in PHP >= 7.1

That would look like this:

if ( false !== strpos( $filepath, '.php', -4 )

Another option, though I'm not sure it's a readability improvement, is using in_array() and substr().

if ( ! in_array( substr( $filepath, -4 ), array( '.php', '.PHP' ), true ) )

#6 @kirasong
6 years ago

Thanks so much both!

@desrosj Nice! I wish we could use the method that is 7.1+. Maybe when WordPress starts requiring it.

The in_array() pass you wrote is a bit faster in my testing. I think it's easier to read than the substr_compare() approaches. It doesn't catch *quite* as many files, although I'm guessing not very many are named .Php.

Since it seems we all liked 50784.substr.diff​ and it lines up with some existing methods in core (thanks, @SergeyBiryukov!), I think it makes sense to go ahead and do that for now.

If we see issues with performance, we could come back to the in_array() approach, perhaps?

As we're near RC, I'll get that taken care of for now, and we can iterate if y'all would like.

Last edited 6 years ago by kirasong (previous) (diff)

#7 @kirasong
6 years ago

  • Owner set to mikeschroder
  • Status new → assigned

#8 @kirasong
6 years ago

  • Resolution → fixed
  • Status assigned → closed

In 48657:

Upgrade/Install: Improve wp_opcache_invalidate() performance.

Changes from using preg_match() based .php extension checking to using substr() in wp_opcache_invalidate().

Props jnylen0, aaroncampbell, SergeyBiryukov, desrosj, mikeschroder.
Fixes #50784.

#9 @kirasong
6 years ago

  • Focuses performance added
  • Keywords commit removed
Note: See TracTickets for help on using tickets.