WordPress.org

Make WordPress Core

#41501 closed defect (bug) (invalid)

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

Reported by: pento Owned by: 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 23 months ago.
41501.patch (655 bytes) - added by umangvaghela123 23 months ago.
Your solution is good but it is fine if we use this code standard.
41501.2.diff (661 bytes) - added by pento 23 months ago.
41501_2.patch (652 bytes) - added by umangvaghela123 23 months ago.
41501_3.patch (649 bytes) - added by umangvaghela123 23 months ago.
strpos() function is working wrong.
41501_4.patch (701 bytes) - added by umangvaghela123 23 months ago.
Your solution is good but it is strpos() condition is wrong.

Download all attachments as: .zip

Change History (17)

#1 @pento
23 months ago

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

@pento
23 months ago

#2 @pento
23 months 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
23 months ago

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

@umangvaghela123
23 months ago

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

@pento
23 months ago

#4 @pento
23 months 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
23 months 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
23 months ago

strpos() function is working wrong.

#6 @umangvaghela123
23 months ago

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

@umangvaghela123
23 months ago

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

#7 follow-up: @pento
23 months 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
23 months 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.


23 months ago

#10 @westonruter
23 months ago

  • Description modified (diff)

#11 @pento
23 months 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.