Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#41501 closed defect (bug) (invalid)

Emoji: Short-circuit wp_staticize_emoji() when there are no emoji to staticise.

Reported by: pento's profile pento Owned by: pento's profile pento
Milestone: Priority: high
Severity: major Version: 4.9
Component: Emoji Keywords: has-patch needs-testing
Focuses: performance Cc:

Description (last modified by westonruter)

After [41043] (for #35293), there seem to be some circumstances where the new regex is noticeably slower, particularly with PHP < 7, and PCRE < 8.39.

This isn't noticeable for most sites, but does become a problem for sites with a particularly high volume of HTML email. (Looking at you, WP.com.)

To help mitigate that, we can short circuit the process with early checks for emoji - if there are none, we can return before running the regex.

Attachments (6)

41501.diff (654 bytes) - added by pento 7 years ago.
41501.patch (655 bytes) - added by umangvaghela123 7 years ago.
Your solution is good but it is fine if we use this code standard.
41501.2.diff (661 bytes) - added by pento 7 years ago.
41501_2.patch (652 bytes) - added by umangvaghela123 7 years ago.
41501_3.patch (649 bytes) - added by umangvaghela123 7 years ago.
strpos() function is working wrong.
41501_4.patch (701 bytes) - added by umangvaghela123 7 years ago.
Your solution is good but it is strpos() condition is wrong.

Download all attachments as: .zip

Change History (17)

#1 @pento
7 years ago

  • Owner set to pento
  • Status changed from new to assigned

@pento
7 years ago

#2 @pento
7 years ago

  • Keywords has-patch needs-testing added

I'm going to do some performance tests with this patch, any further suggested changes would be appreciated.

#3 @pento
7 years ago

Running performance tests here: https://travis-ci.org/pento/test-41501

@umangvaghela123
7 years ago

Your solution is good but it is fine if we use this code standard.

@pento
7 years ago

#4 @pento
7 years ago

Thank you for the additional patch, @umangvaghela123!

Unfortunately, 41501.patch doesn't quite do the right thing - I've created 41501.2.diff, to show a little clearer what's going on.

It can return early, if the following conditions are met:

  • It only contains ASCII. This is tested by mb_check_encoding(), if available, or the preg_match(), if it isn't.
  • AND, it doesn't contain the string '&#x', which possibly indicates a HTML encoded entity that we need to test further.

#5 @umangvaghela123
7 years ago

@pento Thank you for your response.

If we check ASCII so it is fine to check check_ascii() which is already defined in wp-include/wp-db.php file.

@umangvaghela123
7 years ago

strpos() function is working wrong.

#6 @umangvaghela123
7 years ago

Test the code and find strpos() function is not mentioned properly.

@umangvaghela123
7 years ago

Your solution is good but it is strpos() condition is wrong.

#7 follow-up: @pento
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting this to 4.9, as I can't find a good improvement, and 4.8.1 needs to go ahead.

#8 in reply to: ↑ 7 @westi
7 years ago

Replying to pento:

Punting this to 4.9, as I can't find a good improvement, and 4.8.1 needs to go ahead.

Seeing as we now know the new code has issues on large content should we not remove the existing changes to the RegEx from 4.8.1 so that we don't potentially cause issues on other sites with this Maintenance Release?

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


7 years ago

#10 @westonruter
7 years ago

  • Description modified (diff)

#11 @pento
7 years ago

  • Milestone 4.9 deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

Well, this was a fun thing to wake up to! ;-)

Given that the whole thing has been reverted from 4.8.1, I'm going to close this ticket and continue developing on #35293.

Note: See TracTickets for help on using tickets.