Make WordPress Core

Opened 10 years ago

Closed 6 years ago

Last modified 21 months ago

#26638 closed enhancement (wontfix)

Performance Increase in l10n (9-12%)

Reported by: dshafik's profile dshafik Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: I18N Keywords: has-patch
Focuses: performance Cc:

Description (last modified by ocean90)

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 10 years ago.
26638.patch (2.5 KB) - added by SergeyBiryukov 10 years ago.
l10n-false.patch (1.2 KB) - added by dshafik 10 years ago.
26638.2.patch (2.5 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (25)

@dshafik
10 years ago

#1 follow-up: @nacin
10 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
10 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
10 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
10 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
10 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
10 years ago

#6 @dshafik
10 years ago

Benching all three, I see the following:

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

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

#7 @nacin
10 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
10 years ago

26638.2.patch avoids a function call.

#9 in reply to: ↑ 8 @dshafik
10 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
10 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
10 years ago

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

#12 @dshafik
10 years ago

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

#13 @gurudrew
10 years ago

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

#14 @markoheijnen
10 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
10 years ago

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

#16 follow-up: @SergeyBiryukov
10 years ago

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

#17 in reply to: ↑ 16 @Tklaversma
9 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
9 years ago

  • Keywords needs-testing added; 4.1-early removed

#19 @swissspidy
7 years 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.

#20 @ocean90
6 years ago

  • Description modified (diff)
  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Going to close this as a wontfix for now since the patches don't comply with how core loads translations anymore. See #34114 for history.
Feel free to reopen if someone has a solution which doesn't copy most of get_translations_for_domain() into each translate function and still has a noticeable performance improvement.

#21 @SergeyBiryukov
21 months ago

  • Keywords needs-testing reporter-feedback dev-feedback removed

Found this ticket again while looking at #26640.

It looks like the NOOP_Translations usage was improved in [36538] / #21319, where we save an instance of the class to a static variable instead of creating a new instance every time, as originally implemented in [12080] / #10971.

Out of curiosity, I've experimented a bit more with something along the lines of:

if ( isset( $l10n[ $domain ] ) ) {
	$translations = $l10n[ $domain ];
} else {
	$translations = get_translations_for_domain( $domain );
}

if ( $translations instanceof Translations ) {
	$translation = $translations->translate( $text );
} else {
	$translation = $text;
}

But that did not provide any noticeable performance improvement, at least not on PHP 8.0.x. So I guess these particular micro-optimizations are no longer relevant here.

Note: See TracTickets for help on using tickets.