Last modified: 2011-11-07 17:51:31 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T31283, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 29283 - Localisation Cache ACCEL store
Localisation Cache ACCEL store
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Internationalization (Other open bugs)
unspecified
All All
: Lowest enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: i18n, patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-06 14:49 UTC by Vitaliy Filippov
Modified: 2011-11-07 17:51 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
LCStore_ACCEL implementation patch (2.23 KB, patch)
2011-06-06 14:49 UTC, Vitaliy Filippov
Details
Updated patch for r101353 (2.92 KB, patch)
2011-10-31 12:09 UTC, Vitaliy Filippov
Details
Check preload to recache (716 bytes, patch)
2011-11-07 12:17 UTC, Vitaliy Filippov
Details

Description Vitaliy Filippov 2011-06-06 14:49:00 UTC
Created attachment 8625 [details]
LCStore_ACCEL implementation patch

Why there's no LCStore_ACCEL backend for LocalisationCache ? Isn't it irrational to use DB for it ?
Attached patch with an implementation that we use in our MW.
Comment 1 Niklas Laxström 2011-06-06 14:53:32 UTC
Seems waste of memory to me, but I think Tim should comment.

The patch has some code style issues.
Comment 2 Sam Reed (reedy) 2011-06-06 15:37:52 UTC
It's also missing an AutoLoader entry

Also, the "require_once 'ObjectCache.php';" shouldn't be needed


The patch is also not properly targetting a file
Comment 3 Vitaliy Filippov 2011-06-06 19:57:49 UTC
(In reply to comment #1)
> Seems waste of memory to me, but I think Tim should comment.
Can you explain it a little?
Comment 4 Siebrand Mazeland 2011-09-03 16:01:32 UTC
Core devs don't see the use or find things wrong with the patch. This issue will not progress until the reported updates the patch so that it can be applied properly to trunk.
Comment 5 John Du Hart 2011-10-26 15:51:06 UTC
-needs-review

Per the above the diff isn't properly targetting a file and needs an Autoloader entry. Also if you could make it confirm to our Coding Conventions it would be appreciated. https://www.mediawiki.org/wiki/Manual:Coding_conventions

Let me know on #mediawiki if you'd like some help :)

+reviewed
Comment 6 Vitaliy Filippov 2011-10-31 12:09:12 UTC
Created attachment 9322 [details]
Updated patch for r101353

Why it's not properly targetting a file?
Do you mean the path must be 'phase3/includes/LocalisationCache.php' instead of just 'includes/LocalisationCache.php'?
If so, see the updated patch. I've also corrected the code style, added an AutoLoader entry and removed require_once ObjectCache.php, although I remember it didn't work in 1.16 without it :)
Also, there was an issue with accel store - sometimes 'list:messages' expired in XCache while 'deps' didn't, which leaded to warnings on the top of page. I've fixed it - now it checks both keys to determine if the cache is missing.
Comment 7 Sam Reed (reedy) 2011-11-01 07:18:16 UTC
Minor thing, we don't capitalise null
Comment 8 Mark A. Hershberger 2011-11-01 16:11:29 UTC
r101492
Comment 9 Mark A. Hershberger 2011-11-01 18:34:19 UTC
re-applied at r101507 with way to make it work if there is no APC cache installed
Comment 10 Vitaliy Filippov 2011-11-02 14:27:43 UTC
Thanks! So, wfGetCache doesn't return FakeMemCachedClients anymore?
Comment 11 Roan Kattouw 2011-11-02 14:30:06 UTC
(In reply to comment #10)
> Thanks! So, wfGetCache doesn't return FakeMemCachedClients anymore?
I don't know whether it ever did, I'm not familiar with wfGetCache(). But I can tell you what it did on my install :) . I do recall that Tim refactored wfGetCache() and the whole ecosystem around it a while ago, perhaps that's when this change in behavior was introduced.

See ObjectCache::newAccelerator() in includes/objectcache/ObjectCache.php
Comment 12 Vitaliy Filippov 2011-11-03 11:15:29 UTC
In the meantime I've found another bug with the implementation - it also relies on the presence of key '...:preload' and doesn't check it when checking for the expired cache, which can lead to "Invalid or missing localisation cache" exceptions.
I'll fix it soon.
About the comments to r101507:
Maybe LCStore_Accel should force disable manualRecache itself?
Comment 13 Vitaliy Filippov 2011-11-07 12:17:46 UTC
Created attachment 9370 [details]
Check preload to recache
Comment 14 Domas Mituzas 2011-11-07 12:20:25 UTC
back in the day I livehacked in APC-based cache for languages in mediawiki, it definitely makes sense to keep this data in memory ;)
Comment 15 Mark A. Hershberger 2011-11-07 17:51:31 UTC
New patch applied r102304

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links