WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 16 months ago

#26638 new enhancement

Performance Increase in l10n (9-12%)

Reported by: dshafik Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.9
Component: I18N Keywords: has-patch needs-testing reporter-feedback dev-feedback
Focuses: performance Cc:

Description

While doing some writing on profiling I choose Wordpress as my example application and wrote a small patch that in my testing boosts performance of index.php by 9-12% (with the dataset from my 6 year old blog, ~250 posts, lots of plugins and extra stuff).

This performance increase is found by making a small change to the way translate() works, effectively cutting out the use of the NOOP_Translates class.

Patch is attached.

Attachments (4)

l10n.patch (522 bytes) - added by dshafik 4 years ago.
26638.patch (2.5 KB) - added by SergeyBiryukov 4 years ago.
l10n-false.patch (1.2 KB) - added by dshafik 4 years ago.
26638.2.patch (2.5 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (23)

@dshafik
4 years ago

#1 follow-up: @nacin
4 years ago

Thanks for the patch! NOOP_Translations is used when a domain is "loaded" but no translations actually exist for that domain. It is a dead-simple class, essentially functioning as a no-op (as the name says). What would need to happen is get_translations_for_domain() would still need to be called, as this would break translation usage as written. While we could avoid calling NOOP_Translations, we'd still need to check whether the object returned is NOOP_Translations before avoiding the function call. I've looked at trying to squeeze performance out of this before — is it really that much overhead? How did you calculate these numbers?

#2 in reply to: ↑ 1 @dshafik
4 years ago

Replying to nacin:

In this case, when using the domain default — which is the only time I skip it — only the NOOP_Translations is used. Note that I'm still calling the gettext filter, just with the same text that would get returned by NOOP_Translations.

These numbers are from xhprof profiling, I've done it on a couple of machines now, several months apart, we're saving 1500+ function calls on a standard index.php alone (I'm using a based WP install, just my DB), and the instantiation of the object too.

#3 follow-up: @nacin
4 years ago

The domain default is the WordPress domain. See also load_default_textdomain(). If you downloaded, say, the Spanish language pack from es.wordpress.org, nothing will get translated.

The instantiation of the object only happens once, and it's light, so that's not a big deal. I agree that avoiding the function calls would be nice. If it consulted $l10n directly instead of calling get_translations_for_domain() and then did if ( $l10n['default'] instanceof NOOP_Translations ) (and if true, bypassed the second function call), I think we'd get most of the benefits here without sacrificing the translation aspects.

#4 in reply to: ↑ 3 @dshafik
4 years ago

Replying to nacin:

instanceof can be expensive, it would be better to set it to (and check for) false. I can make this change, re-run the profiling to compare and update the patch.

#5 @SergeyBiryukov
4 years ago

We could probably do something like 26638.patch.

is_textdomain_loaded() is basically the same check as in get_translations_for_domain(): tags/3.8/src/wp-includes/l10n.php#L667.

@dshafik
4 years ago

#6 @dshafik
4 years ago

Benching all three, I see the following:

I think the latter is the most comprehensive patch though...

Last edited 4 years ago by dshafik (previous) (diff)

#7 @nacin
4 years ago

The final patch will break all other uses of get_translations_for_domain(), such as _n (translate_plural()), etc. It will also change the return value of get_translations_for_domain(), which is not ideal (but something we can work around if we found a change to the data structure was beneficial).

#8 follow-up: @SergeyBiryukov
4 years ago

26638.2.patch avoids a function call.

#9 in reply to: ↑ 8 @dshafik
4 years ago

Replying to SergeyBiryukov:

26638.2.patch avoids a function call.

I think in the interest of readability the previous patch (26638.patch) is better.

#10 @SergeyBiryukov
4 years ago

  • Version changed from trunk to 2.9

Previously: #10971

Looks like NOOP_Translations was introduced in [12079] and [12080] to prevent duplicated code. Still, a 6% performance improvement without breaking anything sounds good to me.

#11 @ocean90
4 years ago

  • Focuses performance added
  • Summary changed from [Patch] Performance Increase in l10n (9-12%) to Performance Increase in l10n (9-12%)

#12 @dshafik
4 years ago

Will this be landing in 4.0? Between this, and #26640 that would yield a 20-23% performance improvement.

#13 @gurudrew
4 years ago

Nice work Davey! That's a pretty drastic improvement.

#14 @markoheijnen
4 years ago

It's to late for 4.0 but I don't mind to test this on some of my sites. So what is the patch that is now ready to be tested?

#15 @dshafik
4 years ago

@markoheijnen it's attached as 26638.patch, or a less readable suggestion (IMO) 26638.2.patch

#16 follow-up: @SergeyBiryukov
4 years ago

  • Keywords has-patch 4.1-early added
  • Milestone changed from Awaiting Review to Future Release

#17 in reply to: ↑ 16 @Tklaversma
3 years ago

Replying to SergeyBiryukov:

Im curious when this be implemented since we're already on 4.2. The same goes for #26640.

#18 @samuelsidler
3 years ago

  • Keywords needs-testing added; 4.1-early removed

#19 @swissspidy
16 months ago

  • Keywords reporter-feedback dev-feedback added

Now that we have _load_textdomain_just_in_time() (#34114), the approach in the latest patch doesn't seem viable anymore.

@dshafik If you're still interested in this, it would be great to hear your opinion on how improvements could be made now. Also, how did you measure the performance? Makes testing/sharing patches easier.

Also pinging @ocean90.

Note: See TracTickets for help on using tickets.