Make WordPress Core

Opened 7 years ago

Last modified 17 months ago

#41305 assigned enhancement

Add lazily evaluated translations

Reported by: schlessera's profile schlessera Owned by: timothyblynjacobs's profile timothyblynjacobs
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: I18N Keywords: has-patch early dev-feedback needs-testing has-unit-tests
Focuses: rest-api, performance Cc:

Description

In the context of #40988, I did a few performance tests and experimented with adding a lazily evaluated translation object.

The general principle is this:

Instead of returning the resulting string of a translation, return an object for which the __toString() and jsonSerialize() methods will fetch the resulting string instead.

I tested by having the __() method return such a proxy object, instead of the actual translated string.

From a quick profiling run on wptrunk.dev/wp-json/wp/v2/posts, I got the following results:

Returning a translate() from __():

Wall Time     162ms
CPU Time      157ms
I/O Time     5.48ms
Memory       16.5MB
Network         n/a     n/a     n/a
SQL          4.41ms    13rq

Returning a TranslationProxy from __():

Wall Time     144ms   -19ms  -14.9%
CPU  Time     138ms   -18ms  -15.4%
I/O  Time    5.33ms  -154µs   -3.0%
Memory       16.6MB +81.6KB     n/s
Network         n/a     n/a     n/a
SQL          4.33ms    13rq

As you can see, this shaved off almost 15% from this simple request.

It saved 2255 calls to translate(), 2157 calls to get_translations_for_domain() and, more importantly still, 2156 calls to apply_filters() (which could involve a lot of additional processing in some cases).

The main problem with this approach is that WordPress does not contain real type-hinting, so BC is broken wherever the proxy is not echoed, but used directly.

As we cannot possibly foresee how plugins might use their localized strings, I suggest adding new lazy variations of the translation functions. To mirror the "echo" variations that prefix the translation functions with an e, I'd suggest using the l prefix for these variations:

// Lazily retrieve the translation of $text.
_l( $text , $domain = 'default' );

// Lazily retrieve the translation of $text and escape it for safe use in an attribute.
esc_attr_l( $text, $domain = 'default' );

// Lazily retrieve the translation of $text and escape it for safe use in HTML output.
esc_html_l( $text, $domain = 'default' );

// Lazily retrieve translated string with gettext context.
_lx( $text, $context, $domain = 'default' );

// Lazily translate string with gettext context, and escape it for safe use in an attribute.
esc_attr_lx( $text, $context, $domain = 'default' );

// Lazily translate string with gettext context, and escape it for safe use in HTML output.
esc_html_lx( $text, $context, $domain = 'default' );

Arbitrary testing has shown that using such lazily evaluated translations strategically can improve the performance by 10-30% for certain scenarios.

Implementing them in this BC fashion allows us to fine-tune Core usage and make it available to plugins, while playing it safe with existing code.

Attachments (15)

41305.implementation (14.6 KB) - added by schlessera 7 years ago.
Initial implementation. This sets everything up, but does not any benefits yet because it is not being consumed by any other code.
41305.optimize-rest-api (169.5 KB) - added by schlessera 7 years ago.
Optimizes all REST API translation to be lazily evaluated.
41305.implementation.diff (14.6 KB) - added by schlessera 7 years ago.
No changes, just added .diff extension for syntax highlighting.
41305.optimize-rest-api.diff (169.5 KB) - added by schlessera 7 years ago.
No changes, just added .diff extension for syntax highlighting.
41305.optimize-rest-api.2.diff (169.3 KB) - added by schlessera 7 years ago.
Refresh of the REST API practical application
41305.all-in.diff (39.9 KB) - added by schlessera 6 years ago.
Direct change to default translation mechanism
WP trunk default.png (340.4 KB) - added by schlessera 6 years ago.
WP trunk default
WP trunk with 41305.all-in-diff applied.png (339.6 KB) - added by schlessera 6 years ago.
WP trunk with 41305.all-in-diff applied
WP trunk default (curl).png (186.0 KB) - added by schlessera 6 years ago.
WP trunk default (curl)
WP trunk with 41305.all-in-diff applied (curl).png (186.8 KB) - added by schlessera 6 years ago.
WP trunk with 41305.all-in-diff applied (curl)
41305.all-in.2.diff (46.8 KB) - added by schlessera 6 years ago.
Refreshed "all-in" patch, fixed underscore in filename, reordered l10n tests
41305.diff (39.9 KB) - added by TimothyBlynJacobs 6 years ago.
41305.3.diff (46.7 KB) - added by TimothyBlynJacobs 6 years ago.
41305.4.diff (47.4 KB) - added by TimothyBlynJacobs 6 years ago.
41305.5.diff (56.9 KB) - added by TimothyBlynJacobs 6 years ago.

Download all attachments as: .zip

Change History (87)

@schlessera
7 years ago

Initial implementation. This sets everything up, but does not any benefits yet because it is not being consumed by any other code.

@schlessera
7 years ago

Optimizes all REST API translation to be lazily evaluated.

#1 @schlessera
7 years ago

  • Keywords has-patch added

I've now uploaded an initial implementation, as well as a separate patch that replaces all translated strings in the REST API to make use of these lazy-loading proxies, as it think it should be relatively safe to do so for the REST API.

This needed two small tweaks to the unit tests, where the tests were assert the actual internal type of string on error messages. I don't think that the probability is very high that actual code should hit that issue.

This relatively conservative change reduced the wall time on my benchmarks (with 50 iterations) from 162ms to 152 ms, by eliminating 1236 translations.

Of course there's lots of other potential places where we can apply these optimizations as well. But I don't want to pollute this ticket too much.

#3 @jdgrimes
7 years ago

Ultimately, I see the bigger underlying problem as being a design flaw in many of the APIs where the translation functions are used. The ideal would have been to design the code so that it did not have to worry about the translations at all until it actually needed them. However, that is now water under the bridge, and although we could take this into account when designing future code, introducing these lazy functions is probably worth the immediate performance gains.

A better design of the code to start with would likely also decrease memory, whereas the lazy approach actually increases it overall in some cases, although it appears that the memory increase is reasonably small based on the numbers in the OP. I assume that memory consumption would still be only minimally more than it presently is even if the lazy functions were used by most plugins, but that might also be worth some consideration and investigation. In cases where a text domain did not have to be loaded at all (i.e., the translations were never actually used), memory might actually be decreased.

#4 @schlessera
7 years ago

@jdgrimes Yes, I totally agree that a better upfront design would be preferable. But that would mean ripping everything out and breaking the entire ecosystem, pretty much...

This approach is a sort of "opt-in" improvement, which lets us fine-tune the usage by hand without breaking anything, and lets plugins/themes make use of this where it makes sense.

#5 @rmccue
7 years ago

  • Focuses rest-api added

Rather than being too clever with objects, we could potentially just introduce _noop() and translate_noop functions instead, to match the existing _n_noop and translate_nooped_plural. The __noop() is just a static analysis hint, and the actual translation can be deferred until later; we can implement this for the API (the only place we really need it) pretty easily, it just requires us to add the functions to the static analysis.

This would look something like:

public function get_item_schema() {
	// ...
	'translation_domain' => '',
	'properties' => array(
		// ...
		'description' => _noop( "The date the object was published, in the site's timezone." ),
	)
	// ...
}

Later, we then call translate_noop( $description, $translation_domain ) when needed.

This is less surprising to consumers of the function, since it's still actually a string. One of the problems with PHP is that string-like objects aren't equivalent to first-class strings, so introducing an object with __toString() might introduce problems.

In an ideal world, we would have implemented the API so that things like this could either be a string or a function, allowing us to defer the actual runtime of translation:

'description' => function () {
	return __( "The date the object was published, in the site's timezone." );
}

However PHP doesn't fit well with this pattern generally, and we also don't have proper closures in PHP 5.2, so we can't really do this.

#6 @schlessera
7 years ago

I don't consider this "too clever", as I specifically introduced new methods instead of changing old ones, and they are documented to return a proxy object. Using proxy objects is a standard practice, and developers are free to not use these new methods if they don't understand what that means.

What you propose means that you split the translation code up into two different locations for strings translated in this way. For more complex code, you'd basically need to run your strings through a check to make sure that translatable strings was indeed translated. I don't understand how this would be preferable.

There are probably use cases where this makes sense, but as a general mechanism, I think this makes the code even worse, and introduces lots of bugs where we forgot to actually translate a translatable strings. This won't even be detected by any of our automated tests. With the proxy object above, you at least get an immediate error if you handle it incorrectly.

#7 @swissspidy
7 years ago

#38643 is an area where something like this would have been useful. For #38218 specifically I suggested using a new class.

One thing that bugs me about this patch here is that it would make transitioning to a new translation system (still dreaming of MessageFormat) much harder because of all the new functions and classes.

I like keeping it simple with _noop() and translate_noop().


@schlessera Would you mind using .diff file extensions next time so we have syntax highlighting here in Trac? Thanks :-)

#8 @schlessera
7 years ago

One thing that bugs me about this patch here is that it would make transitioning to a new translation system (still dreaming of MessageFormat) much harder because of all the new functions and classes.

I think that this should not be an issue, because the added functions are basically lazy wrappers around translate(). They don't do any translation themselves, and when you would change the backend implementation for translate(), theses lazy variations are taken care of automatically.

I like keeping it simple with _noop() and translate_noop().

I don't think that's simpler. It is simpler code in the implementation, but then makes every single usage of it more complex as a result, as you are splitting up the usage into two separate steps, and are making this much more error-prone.

Would you mind using .diff file extensions next time so we have syntax highlighting here in Trac? Thanks :-)

Uh, sure, I wasn't aware of that distinction. Had mostly avoided Trac before... ;)

@schlessera
7 years ago

No changes, just added .diff extension for syntax highlighting.

@schlessera
7 years ago

No changes, just added .diff extension for syntax highlighting.

#9 @jnylen0
7 years ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to jnylen0
  • Status changed from new to assigned

I think something like this is badly needed (/wp/v2?context=help likely has even worse performance), and the approach in the patches so far is reasonable.

I'll plan to take a closer look for 4.9.

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


7 years ago

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


7 years ago

This ticket was mentioned in Slack in #core-restapi by schlessera. View the logs.


7 years ago

#13 @JPry
7 years ago

  • Keywords needs-refresh added

I think the entire premise of this ticket is great. Translations are the kind of thing that are ubiquitous within WordPress, and yet they definitely need to be more performant. I'm +1 for adding the lazy functions and translation proxies that @schlessera proposed. I could foresee using these functions within personal projects to write more performant code.

When looking at this patch locally, it seems that it no longer applies cleanly to trunk, so it should be refreshed.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


7 years ago

@schlessera
7 years ago

Refresh of the REST API practical application

#15 @schlessera
7 years ago

  • Keywords needs-refresh removed

I refreshed the patch that applies the lazily evaluated translations to the REST API, so that it applies cleanly again.

The actual tranlsation code is still good and doesn't need a refresh.

All tests are passing.

Thanks @JPry for testing the patches!

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


7 years ago

#17 @kadamwhite
7 years ago

  • Keywords early added
  • Milestone changed from 4.9 to Future Release

As much as I'd like this to land, after discussion with @swissspidy and @rmccue we'd like more review from @ocean90 than we have time for today—punting to early in the next release cycle.

@schlessera
6 years ago

Direct change to default translation mechanism

#18 @schlessera
6 years ago

As a follow-up to the discussions I had with some of the core folks at WCUS, I produced a new patch 41305.all-in.diff that skips producing alternate translation functions, and immediately changes the default translation functions to return lazily-translated proxies instead.

Why not to play it safe (as the first patch does)?

The reasoning behind this is that this change has such a big impact on performance that we shouldn't start with a compromise that only makes it opt-in. We should properly investigate the expected amount of problems this might cause to plugins and mitigate any breakage to the best of our abilities, as this will benefit all WordPress users in a substantial way in the long term. The discussions at WCUS led to the conclusion that these are probably only edge cases and that the change is important enough to warrant contacting a few plugin authors that might be impacted.

I added some minimal edge case handling code to make sure unit tests still pass and plugins would just work. However, right now, there are still 7 unit tests that are breaking. For these, I think the unit tests might need to be adapted instead, though.

What are the breaking changes?

The translations are not real strings anymore, but proxy objects that behave like strings whenever being cast to a string (like when being echoed, concatenated, etc...). For most of the intended usage of translations (sending them to templates to get rendered into HTML responses), this just works without issues. However, three scenarios can cause issues with this:

1. Directly checking the type of the translation

This mainly happens in unit tests. That's why I created an override in the WP_Unit_TestCase for the assertEquals(), assertSame() and assertInternalType() methods. Creating such overrides also fixes breaking tests for any plugins that base their tests on WP_UnitTestCase without needing further adaptations.

2. Using the translation as an array index

This is actually done by Core in places like the locale setup. I think that there are not that many use cases for doing so, as you'd prefer deterministic indexes most of the time. The solution to this is to just cast to (string) before actually using the translation as the index.

3. Making the result of the translation dependent on the timing of its instantiation, instead of its usage

I would be surprised to learn that plugins would ever use translations in this way. However, Core unit tests currently do, and this also includes the 7 unit tests that are currently still failing.

The failing unit tests do something like this: add_filter(), __(), remove_filter(), assert(). So, they immediately remove the change they want to test before asserting. They are all part of the Tests_L10n_loadTextdomainJustInTime suite, and I'm not sure they need fixing. If I understand correctly what they are testing, I think the tests should just be adapted to move the remove_filter() calls after the assert() instead of in front of it. I would love to get more insight into this from @ocean90 & @swissspidy, as I think they had collaborated on this feature & tests.

Current results

What follows is a couple of screenshots to show the current results. The tests were done as a call to <domain>/wp-json/wp/v2/posts/1 to retrieve the JSON representation of the Hello World default post. They have been produced on my local system running PHP 7.1.17 with xdebug enabled. I kept xdebug enabled because it makes everything slower, so the change is more obvious. The setup was a fresh and default WP setup with fr_FR as frontend language and de_DE as user language.

Also, please note that the screenshots represent 1 random run, chosen to be somewhat representative. I will add real benchmark results (with averaged values) as soon as I manage to properly configure my system to run them again, as I had to find out this currently does not work.

Wall time without this patch: ~1.6s

Wall time with this patch: ~1.1s

You can also see that the query time is practically unchanged, so this is pure processing logic to make the translations happen (which are not even being used in this case).

@schlessera
6 years ago

WP trunk default

@schlessera
6 years ago

WP trunk with 41305.all-in-diff applied

#19 @schlessera
6 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 5.0

#20 @swissspidy
6 years ago

They are all part of the Tests_L10n_loadTextdomainJustInTime suite, and I'm not sure they need fixing. If I understand correctly what they are testing, I think the tests should just be adapted to move the remove_filter() calls after the assert() instead of in front of it.

The statements are ordered that way so everything gets cleaned up immediately when not being used anymore (e.g. in case of errors) and assertions are nicely grouped together to make things easier to grasp. Shouldn't be a big deal to re-order those.

#21 @schlessera
6 years ago

I think the way I profiled the command above does mess somewhat with the processing.

The following screenshots show another way of timing the changes, using curl to retrieve the same REST API request.

Wall time without this patch: ~0.49s

Wall time with this patch: ~0.35s

@schlessera
6 years ago

WP trunk default (curl)

@schlessera
6 years ago

WP trunk with 41305.all-in-diff applied (curl)

#22 @schlessera
6 years ago

@swissspidy :
Ah, thanks, I already thought so but wanted to make sure I didn't just misunderstand the point of the tests.

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


6 years ago

#24 @mnelson4
6 years ago

FYI I would love to see this happen. I took the latest patch for a spin with our plugin and there were no fatal errors, but it broke some key functionality (our plugin does event registration, and it broke that). We were using translated strings as array keys. There were warnings for both of these, and after I cast them to strings, the code worked fine.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

@schlessera
6 years ago

Refreshed "all-in" patch, fixed underscore in filename, reordered l10n tests

#27 @schlessera
6 years ago

I just uploaded a refresh for the "all-in" patch as was requested by @kadamwhite.

Changes relative to previous patch:

  • Fixed underscore in filename class-wp-contextual_translation-proxy.php => class-wp-contextual-translation-proxy.php
  • Reordered the l10n tests from setup->execution->teardown->assert to setup->execution->assert->teardown, as previously discussed with @swissspidy in https://core.trac.wordpress.org/ticket/41305#comment:20

#28 @kadamwhite
6 years ago

  • Keywords needs-testing added

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


6 years ago

#30 @TimothyBlynJacobs
6 years ago

I think the way the lazy property is computed could be changed. My understanding is that using an uninitialized property forces PHP to allocate a hash table for the object which takes more memory than having just a declared list of properties.

When I benched the difference there was a considerable difference in memory.

Uninitialized Property

  • Time: 10.40907907486
  • Memory: 550,057,104

Declared Property

  • Time: 9.9305930137634
  • Memory: 174,057,104
<?php
$s_time = microtime( true );
$s_mem  = memory_get_usage();

for ( $i = 0; $i < 1000000; $i ++ ) {
        $$i = __( 'My String' );
        (string) $$i;
}

$d_time = microtime( true ) - $s_time;
$d_mem  = memory_get_usage() - $s_mem;

echo $d_time . PHP_EOL;
echo $d_mem . PHP_EOL;

I didn't notice any consistent difference between the two methods when repeatably accessing the same lazy string.

( I'm not super familiar with benchmarking, but I think this is rightish. )

https://gist.github.com/nikic/5015323

#31 @mnelson4
6 years ago

What @TimothyBlynJacobs is saying makes sense. I don't see any reason why WP_String_Proxy doesn't just declare the property $result (instead it unnecessarily uses the magic method __get() and sets the property dynamically).

However, is your patch right @TimothyBlynJacobs? When I apply that patch to master I get a fatal error because the file class-wp-string-proxy.php is missing.

Also, I tried to double-check your benchmark but couldn't because of the above exception.

#32 @TimothyBlynJacobs
6 years ago

Sorry about that! Uploaded what should be a correct version.

#33 @mnelson4
6 years ago

I ran @TimothyBlynJacobs's test, (except with one tenth the iterations because I was running out of memory).

Summary

41305.3.diff (latest from @TimothyBlynJacobs) is nearly twice as fast as
41305.all-in.2.diff​ (latest from @schlessera) and uses 1/3 the memory.

Details

master:
Time 0.23000001907349
Memory: 9490200

41305.all-in.2.diff​ (latest from @schlessera):
Time 0.62402606010437
Memory: 57761176

41305.3.diff (latest from @TimothyBlynJacobs):
Time 0.28000116348267
Memory: 20161176

So, in this test, 41305.3.diff was nearly as fast as master, although used up twice the memory. 41305.all-in.2.diff​ was 3x slower and used up nearly 6x the memory. (Given, this test assumes all strings are evaluated; I realize the reason this ticket helps is because most strings are NOT evaluated).

So 41305.3.diff looks like an improvement on 41305.all-in.2.diff to me.

Note I'm on PHP 7.1.19

#34 @schlessera
6 years ago

Good catch, @TimothyBlynJacobs ! I haven't looked into any micro optimizations yet, as I wanted to find out first whether the basic approach is viable with plugins. But I agree with your analysis, we should use a predefined property.

#35 @ottok
6 years ago

Another optimization to gettext has also been proposed (with patch) in #17268 and it would complement this nicely. Please take a look at that after this one is in.

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

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


6 years ago

#37 @leewillis77
6 years ago

I've just set up a fresh site (from SVN) to try this out. I checked out trunk from SVN, applied 41305.3.diff, and then tried to install.

Unfortunately it's been a while since I ran WordPress from an SVN checkout, and I'd not set things up properly (pointed my document root at src/, not build/, and not done the npm install && grunt build step).

What should have happened in this case, was that I should have seen the screen saying "You seem to be running WordPress from the src directory. WordPress needs to be built and run from the build directory before we can get started." etc.

However, instead I got the following stack trace:

PHP Fatal error:  Uncaught Error: Class 'WP_Translation_Proxy' not found in /Users/lee/sites/lazy-repo/trunk/src/wp-includes/l10n.php:203
Stack trace:
#0 /Users/lee/sites/lazy-repo/trunk/src/wp-includes/class-wp-locale.php(121): __('Sunday')
#1 /Users/lee/sites/lazy-repo/trunk/src/wp-includes/class-wp-locale.php(104): WP_Locale->init()
#2 /Users/lee/sites/lazy-repo/trunk/src/wp-includes/load.php(1018): WP_Locale->__construct()
#3 /Users/lee/sites/lazy-repo/trunk/src/index.php(25): wp_load_translations_early()

#38 @TimothyBlynJacobs
6 years ago

@leewillis77 Could you give this latest patch a try?

I loaded the proxy files in wp_load_translations_early and also adjusted the load order of the proxy files to be loaded right after the rest of the locale files are loaded in wp-settings.php.

I also fixed an issue with wp_die() which casted everything to a string, but WP_Error is an allowed parameter type. For safety, I just changed the string casting to only take place on WP_String_Proxy objects.


Side note. This doesn't entirely fix the issue for the REST API locale ticket because string proxies that have already been evaluated will not have their evaluation dropped when the locale switches.

What do people think about adding a $locale property to the string proxy and changing the result() conditional to: null === $this->result || get_locale() !== $this->locale? I think that should be more memory efficient than allocating an array to store a map of locales => evaluated strings.

#39 @TimothyBlynJacobs
6 years ago

These tests might also be failing

Failed asserting that WP_Translation_Proxy Object &000000006ab8b8c3000000002771c2ce (
    'text' => 'Username contains invalid characters.'
    'domain' => 'default'
    'result' => null
) is of type "string".
tests/phpunit/tests/rest-api/rest-users-controller.php:1143
 

Failed asserting that WP_Translation_Proxy Object &000000006ab8b633000000002771c2ce (
    'text' => 'Sorry, that username is not allowed.'
    'domain' => 'default'
    'result' => null
) is of type "string".
 /tests/phpunit/tests/rest-api/rest-users-controller.php:1183

#40 @TimothyBlynJacobs
6 years ago

Ignore the unit tests comment, was an issue with my local setup.

However, looking at the issue with string proxy evaluations. Following an approach like I describe isn't workable due to #37997.

Braindump of possible solutions:

  1. Always retrieve a fresh value if is_locale_switched(). This would lose a lot of performance gains when doing a REST API request with the locale set.
  2. Move storage to a static array. When locale is switched, empty out the static array. Would require a new protected $id property to use as a key could be based off of the text and any other modifiers, or just a static counter should work. When the object is __destructed it could remove its entry from the static array.
  3. Similar as 2, but change the storage format to be first keyed by locale, then keyed by the proxy ID. Store the current locale as a static property. When the locale changes, change the static locale property. This could have a negative memory impact if switching to a lot of different locales during a request. For instance, when sending out an email to multiple different users. However, I'd imagine that not many different strings would be evaluated in a case like this.

Other thoughts?

#41 @leewillis77
6 years ago

@TimothyBlynJacobs I can confirm that 41305.4.diff resolves the issues I experienced.

#42 @pento
6 years ago

#38218 was marked as a duplicate.

#43 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#44 @mnelson4
6 years ago

FYI I tested @TimothyBlynJacobs's latest patch with a a half dozen of our plugins' add-ons, and they worked fine without modification.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

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


6 years ago

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

#50 @kadamwhite
6 years ago

I would very much like to see this land in 5.1 or 5.2; we discussed with @schlessera and @TimothyBlynJacobs in slack today (see log link above).

To summarize that discussion (and tl;dr the above comments), one remaining issue is what we might break if we return objects from translation functions. This predominately impacts areas where translations are being used as array keys, and tests where plugin authors may be passing translations to assertString(). Neither is unsolvable but returning objects does represent a back-compatibility break. We'll be checking in on this one weekly during the 5.1 cycle to keep things moving.

#51 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

With the 5.1 beta happening tomorrow, I don't have enough time to review this properly. I'm happy for it to land in 5.2 early, though.

This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.


6 years ago

#53 @kadamwhite
6 years ago

  • Owner changed from jnylen0 to timothyblynjacobs

Assigning to @TimothyBlynJacobs to implement a way to bust the cache when locale changes within a request.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

#55 @TimothyBlynJacobs
6 years ago

I've added a patch that stores the translations in a central cache object so they can be changed if the locale switches.

It doesn't bench as well, probably because of the additional property. This might be solvable with spl_object_hash since when the object is destructed, we clear out the value from the centralized cache, so there shouldn't be a risk of PHP reusing the same object handle. This would have to happen after the PHP version bump I think because SPL isn't currently required by core.

11.66620516777
227,797,672

As an aside, now that we solved translations for REST requests differently, I'm not sure how needed the centralized cache busting is. It will make switching locales work better for plugins that don't re-register their translations when the locale changes. Without the centralized cache clearing, and just the lazy evaluation, if a translations wasn't actually evaluated until the locale is switched, then the translation would be incorrect for the rest of the request.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

#57 @mnelson4
6 years ago

I'm not sure how needed the centralized cache busting is

Yeah this is already an improvement on trunk's current approach of translating immediately. And centralized cache busting seems like it could be added later if we still identify the need, right?

Once this lands in 5.2 it would probably be good to try to pre-emptively test this with the most popular plugins/themes (a bit like @danielbachhuber did pre-Gutenberg , just on a much smaller scale). I can put some time towards that.

#58 @TimothyBlynJacobs
6 years ago

I wanted to summarize what I think the most likely candidates for breakage are with the current patch version. This is mainly repeating @schlessera comment in #18.

Strict comparisons with translated strings will no longer work as expected: https://3v4l.org/6CNqk

This pattern is used in core for locale specific "settings". For instance:

// In get_comments_number_text()
'on' === _x( 'off', 'Comment number declension: on or off' )

// In twentyseventeen_fonts_url()
$libre_franklin = _x( 'on', 'Libre Franklin font: on or off', 'twentyseventeen' );

if ( 'off' !== $libre_franklin )

The second area is using lazy translated strings as array keys. For instance, WP_Locale::init(). Unfortunately, PHP doesn't coerce those objects into string array keys. https://3v4l.org/MRCuk

For anyone using strict types, it will also cause a fatal error: https://3v4l.org/gU06F


I'm not sure whether this should be considered an acceptable level of BC breakage. If not, I think we should reconsider using _l and esc_attr_l etc...

This ticket would've solved the translating REST API requests issue, but that has been solved separately.

Additionally, lazily translating every string isn't necessarily a performance boost. A string that is lazy requires an object instantiation. For more complex cases like esc_html__() and esc_attr__() it requires two object instantiations. Additionally there is a memory allocation in the cache pool, and a destructor routine that runs to clear the pool in an attempt to reduce total active memory. For strings that will get rendered or converted to a string in a request, this instantiation is wasted. I think more profiling is needed for how this impacts non REST API requests.

If we were to use a specific lazy function, I imagine we'd change all the schema translations to be lazy loaded.


Some other thoughts:

Lazy translation functions would be easy to "polyfill" for later versions by just making them aliases for their __ variants. So this wouldn't necessarily significantly hamper plugin adoption. There wouldn't be the performance benefits of course, but plugins could use it without having to do version checks all over the place.

Also, now that we have wp i18n, adding new functions to be extracted for gettext might be less of an issue.


If we do think this level of breakage is ok, then I think we want to commit this early and get feedback that way. The different callouts we've done have not gotten a lot of attention. We might have missed that window for 5.2 though.

#59 @mnelson4
6 years ago

Thanks for the thought you put into this, @TimothyBlynJacobs.

I'm not sure whether this should be considered an acceptable level of BC breakage. If not, I think we should reconsider using _l and esc_attr_l etc...

IMO this would be a great middleground.

Additionally, lazily translating every string isn't necessarily a performance boost.

So there is no point in lazily evaluating some strings (one that we know for certainty will be evaluated). So, it might be handy for client code to have the option to have the option to choose either __ or _l.

Lazy translation functions would be easy to "polyfill" for later versions by just making them aliases for their variants.

Are you suggesting we'd add _l in a patch to 5.1.x, for example? Except it would actually only be a wrapper for __? I think that would be good. I'd still be inclined to do a version check though, to at least make sure my code is executing on a version of WP that has those "pollyfill" versions of _l, but only do that on plugin activation or something (not before every call to _l).

IMO, as much as I like the current implementation, I think it's too much of a breaking change. Adding _l sounds good though.

#60 @TimothyBlynJacobs
6 years ago

Are you suggesting we'd add _l in a patch to 5.1.x, for example?

Backporting could be a possibility, but I meant plugins including their own shims.

#61 @mnelson4
6 years ago

Backporting could be a possibility, but I meant plugins including their own shims.

Oh right, ya just letting plugins have their own shims would be best. They kinda need to do that anyway, even with backporting.

#62 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.2 to 5.3

Missed the 5.2 Beta 1 deadline, moving to 5.3.

#63 @mnelson4
6 years ago

Oh, one alternative to adding _l(), _le(), _l_esc_html() etc would be to add an extra parameter onto existing translation functions that indicates to translate lazily (eg __($text, $text_domain =‘’ , $lazy = false)). No new translation functions, and we keep the number of them from growing exponentially.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

#67 @kadamwhite
5 years ago

@joehoyle is going to open a new ticket to capture a quicker win of caching the `get_item_schema` results; we'd still have to translate a few hundred strings, but only once.

We can then de-milestone this from 5.3 until we can sync back up on the lazy string / _noop questions, especially since the PHP version issue blocking the use of closures have been mercifully removed by the efforts of the servehappy team :)

#68 @kadamwhite
5 years ago

  • Milestone changed from 5.3 to Future Release

The ticket to cache item schema in REST controllers has been opened as #47871. Removing this from 5.3 per comments above.

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


4 years ago

This ticket was mentioned in Slack in #core-i18n by sabernhardt. View the logs.


20 months ago

This ticket was mentioned in PR #4444 on WordPress/wordpress-develop by @swissspidy.


17 months ago
#71

  • Keywords has-unit-tests added

Tries to apply the latest patch from 2019 to current code base.

Trac ticket: https://core.trac.wordpress.org/ticket/41305

@spacedmonkey commented on PR #4444:


17 months ago
#72

I am seeing this be around 2% slower. :(

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/9f97b386-67ea-46ad-9906-0a42eb2e3e6a
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/237508/67d47d0f-c7ff-4026-b3db-c8cc9c11885b

Note: See TracTickets for help on using tickets.