Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MessageCache to improve label lookup performance #25

Merged
merged 1 commit into from
Apr 19, 2014

Conversation

mwjames
Copy link
Contributor

@mwjames mwjames commented Feb 25, 2014

Each single wfMessage call costs around 23ms (using Xdebug profiling) which means that when all properties are registered, wfMessage accounts for up-to 2/3 of the overall the processing/computing
(600ms+) time.

This PR introduces a simple MessageCache (wfMessage uses itself a MessageCache but still runs with 20-35ms per call) which allows to minimize the registration time to under 50ms (~0.5 per property).

As soon as the content language changes or the json file is being modified the cache becomes invalid.

@mwjames mwjames added this to the SESP 1.1 milestone Feb 25, 2014
@mwjames
Copy link
Contributor Author

mwjames commented Feb 25, 2014

@kghbln Feel free

@JeroenDeDauw
Copy link
Member

@mwjames How about using i18n to get rid of some of the design problems of the MW stuff and SimpleCache or something similar for the caching? Having a caching i18n thing in the i18n lib would be neat, as it could then tackle this slowness issue for other extensions as well.

@mwjames
Copy link
Contributor Author

mwjames commented Feb 26, 2014

How about using i18n to get rid of some of the design problems

Mmm... I don't think we would gain much because I would still need the MessageCache class as it describes the particular condition associated with SESP ( contentlanguage + file modification date as cache invalidator). Furthermore we use the file input as fixture to limit any access operations (we only have one read and if necessary one write operation to the cache).

Maybe the SimpleCache would be useful if further encapsulation was required but the whole cache handling is resolved by the MessageCache class without leaking any interface details to the outside (well you can inject a BagOStuff object but we only do this for testing otherwise we don't really care and let the MessageCache handle it).

I can't really see the i18n issue here in SESP because all we do is a simple wfMessage( $key )->inLanguage( $this->language )->text(); (I would have to inject i18n with an additional RequestContext::getMain() which we don't really need) and with or without i18n, we would need a MessageCache class. Using i18n within the current context would only increase the complexity by introducing additional dependencies.

Having a caching i18n thing in the i18n lib

Not sure how you want to handle this, in case of SESP you have a fixture (which is the file that contains the expected keys), if you don't have such fixture and only work on loose keys you may end up making endless read/write operations to the cache (which I think is what MW's MessageCache does and that's why you have such poor performance) or you have to fight possible cache fragmentation.

Normally it doesn't matter but in case of SESP you have 50+ registrations in one go and suddenly the poor performance of wfMessage strikes like a lightning bolt (well if you are WMF and you use a Varnish/Redis cache it probably doesn't matter but if we use the simplest of all SqlBagOStuff which is reliable and works for anyone even without extra tam-tam it matters).

The reason why we can increase performance (citing Xdebug) is because we load all expected keys at once and only refresh the cache at the very end if necessary and in between it is handled like a normal in-memory array lookup.

@mwjames mwjames mentioned this pull request Mar 22, 2014
@mwjames
Copy link
Contributor Author

mwjames commented Apr 19, 2014

SESP\Tests\PropertyRegistryTest::testPropertyWithInvalidType
Undefined offset: 9999

solved by SMW::core SemanticMediaWiki/SemanticMediaWiki@a7db203

mwjames added a commit that referenced this pull request Apr 19, 2014
Use MessageCache to improve label lookup performance
@mwjames mwjames merged commit 1908f0a into master Apr 19, 2014
@mwjames mwjames deleted the message-cache branch April 19, 2014 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants