Make WordPress Core

Opened 9 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#32052 closed enhancement (wontfix)

Add out of the box support for MO file caching

Reported by: nicofuma's profile nicofuma Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: I18N Keywords: needs-refresh has-patch
Focuses: performance Cc:

Description (last modified by SergeyBiryukov)

While profiling WordPress with Blackfire we found that parsing an mo file is a heavy task, and it's done on each page.

Parsing of a .mo file

We propose to use the Object Cache API to not re-parse the file if it hasn't changed.
This way any non English installations with a plugin implementing the Object Cache API could benefit of a significant performance improvement.

You can see here a comparison showing the performance improvement:
comparison with the proposed patch

We found that a few plugins are doing the same thing but they are not used very much and we think it could be great to have it mainstream.

Attachments (5)

04222015-1020_mo_caching.diff (1.1 KB) - added by nicofuma 9 years ago.
proposal
default-mo.png (18.8 KB) - added by nicofuma 9 years ago.
Parsing of a .mo file
mo-cache-comp.png (19.4 KB) - added by nicofuma 9 years ago.
comparison with the proposed patch
04222015-1119_mo_caching.diff (1.3 KB) - added by nicofuma 9 years ago.
Updated patch to avoid any notice
04222015-1551_mo_caching.diff (829 bytes) - added by nicofuma 9 years ago.
patch moved to load_textdomain()

Download all attachments as: .zip

Change History (33)

@nicofuma
9 years ago

proposal

@nicofuma
9 years ago

Parsing of a .mo file

@nicofuma
9 years ago

comparison with the proposed patch

#2 @nicofuma
9 years ago

I just noticed a small typo in the description, I forgot a 'not' in the last sentence:

We found that a few plugins are doing the same thing but they are not used very much and we think it could be great to have it mainstream.

#3 @SergeyBiryukov
9 years ago

  • Description modified (diff)

@nicofuma
9 years ago

Updated patch to avoid any notice

#4 @nicofuma
9 years ago

Just to be a little more precise, these 40ms represent about 30% of the wall time.

Sergey: thanks for the description

Update: We published a blog post about this issue.

Last edited 9 years ago by nicofuma (previous) (diff)

#5 @Rarst
9 years ago

Discussion point — should it be only object cache or should it just use Transients API? Would caching this kind/amount of data in database with transient be beneficial in absence of object cache in installation?

#6 @ocean90
9 years ago

Hello nicofuma,

thanks for your report and the patch.
The PO/MO libraries are used in multiple projects and shouldn't have any WordPress dependencies. So the right place for this is probably load_textdomain().
It's also worth to mention that only a small group of users will benefit of this, since there is no persistent object cache per default.

@nicofuma
9 years ago

patch moved to load_textdomain()

#7 @nicofuma
9 years ago

I missed that the PO/MO libraries shouldn't have any WordPress dependencies, sorry.

Here is a new version of the patch moved to load_textdomain().
I also moved from the Object Cache API to the Transient API and here are the profiles:

Current state: profile
Without any Object Cache: profile comparison with current
With W3 enabled (and its disk Object Cache strategy): profile comparison with current

To sump-up: caching this kind/amount of data in database is beneficial (from 47ms to 25ms). Enabling the Object Cache makes things even faster (from 47ms to 8ms).
Thanks to the transient API we use automatically the best caching strategy (db versus object cache).

Last edited 9 years ago by nicofuma (previous) (diff)

#8 @nicofuma
9 years ago

  • Keywords has-patch added

#9 @nicofuma
9 years ago

Hi,

Just wanted to know if the last patch is good enough or if something else should be changed.

#10 @dd32
9 years ago

Personally I'm a little wary of storing the parsed MO files into transients, as pointed out, that means most users will get the data shifted to the Database instead (So it trades CPU+Disk for Memory+network bandwidth - MySQL & Object Caches are often not on the same machine)

It also means that the memory usage of PHP related to the strings will double (as it'll be stored within the object cache global, as well as in the MO reader), that doesn't seem like much, but it quickly adds up in a localised install.

There's a few people interested in the performance of localised installs (myself included), so you can be sure that your patch will be reviewed, but sometimes it can take a bit of time while all the options are weighed up.

#11 @nicofuma
9 years ago

I perfectly understand it can take time. It's just that if there is some concerns (or other options) I'll be happy to discuss about them :)

In my language (in french) it represents around 1.6MB in the database (without compression) and 2.5MB in PHP. It's not that much but it's not negligible either. That's why I initially proposed to use the Object Cache API instead of the transient but the performance improvement is still noticeable when it is stored in the database.

Note: I just saw that the links in the description are wrong. If someone want to see the hole data:

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


8 years ago

#14 @swissspidy
8 years ago

  • Keywords needs-patch added; has-patch removed

Transients aren't really the solution to this. What about just using wp_cache_get() / wp_cache_set() and therefore leveraging an external object cache if configured?

Also, it's likely that Ginger-MO will already improve performance quite a bit.

#15 @PerS
8 years ago

@swissspidy, I agree and suggest this is merged with #34114

Last edited 8 years ago by ocean90 (previous) (diff)

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


8 years ago

#17 @ocean90
8 years ago

  • Milestone changed from Awaiting Review to Future Release

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


7 years ago

#19 @Adrian2k7
4 years ago

Still a problem many years later :(

#20 @Clorith
2 years ago

It would be interesting to see what kind of impact using transients would have here, as noted it would add a bit more data to normal transfer instead of CPU time, but given that transients will leverage an object cache if it exists, instead of the database, is there a possibility that this may provide a small gain for the majority, and an even greater gain for those with a proper object cache in place?

#21 @adamsilverstein
20 months ago

  • Milestone set to 6.1

Milestoning this for 6.1, this would be a great improvement to land in this release

#22 @adamsilverstein
20 months ago

Based on the comment above from @swissspidy, this should probably be refreshed to use object_cache vs. transient API. marking as needs-refresh.

#23 @adamsilverstein
20 months ago

  • Keywords needs-refresh has-patch added; needs-patch removed

#24 follow-up: @swissspidy
19 months ago

I think setting the milestone is a bit optimistic/ambitious since there is no updated patch nor consensus that this is the best path forward.

Also quoting this Slack thread:

  • Caching was really not worth it either, as plain text files were often faster.
  • If you want to really gain performance, and benefit from system caching, drop .mo files and move to a .php file instead. (this approach was also suggested by nacin in https://core.trac.wordpress.org/ticket/17268#comment:57)
  • “Caching file contents” is not always faster, it might be faster in some environments, but when the Object Cache or database server is not on the same machine, or is under increased load (like in many shared environments) it’s actually much slower than local files.
  • For environments with lots of translations loaded, it can also add significant extra network traffic pulling all the translation strings from a remote server, when they’re already cached locally on the servers ram - where the .mo parsing is actually not at all bad.

I'd say this needs a more active discussion & tests with different implementations before jumping into implementation.

#25 @swissspidy
19 months ago

#56051 was marked as a duplicate.

#26 in reply to: ↑ 24 @desrosj
19 months ago

  • Milestone changed from 6.1 to Future Release

Replying to swissspidy:

I think setting the milestone is a bit optimistic/ambitious since there is no updated patch nor consensus that this is the best path forward.

I'd say this needs a more active discussion & tests with different implementations before jumping into implementation.

I agree with this. There still needs to be more discussion. Since beta 1 is in a few days and there are no patches on modern trunk, I'm going to punt this back to Future Release.

#27 follow-up: @swissspidy
4 weeks ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

With the new i18n system in WordPress 6.5 this is likely a non-issue now as it's much faster than the old one, and supports PHP translation files.

#28 in reply to: ↑ 27 @PerS
4 weeks ago

Replying to swissspidy:

With the new i18n system in WordPress 6.5 this is likely a non-issue now as it's much faster than the old one, and supports PHP translation files.

Agree :)

Note: See TracTickets for help on using tickets.