Page MenuHomePhabricator

ObjectCache changes (BagOStuff->add) in 1.20.x break XCache 3.x support
Closed, ResolvedPublic

Description

Author: bm

Description:
I recently faced the exact same problem as described in this old Maillist thread:
http://www.gossamer-threads.com/lists/engine?do=post_view_printable;post=307511;list=wiki

The problem is, that MediaWiki in the 1.20.x Branch with at least XCache 3.x (didn't test another XCache version) takes more then 10 seconds to process every request. The reason for that is a loop with a 10 seconds timeout within "MessageCache::load":

...
Article::view 1 10064.915 10064.915 99.300% 1640737 ( 10064.915 - 10064.915) [291]
MessageCache::load 1 10037.354 10037.354 99.028% 1606 ( 10037.354 - 10037.354) [1]
OutputPage::output 1 43.939 43.939 0.434% 1838602 ( 43.939 - 43.939) [583]
Output-skin 1 43.630 43.630 0.430% 1827387 ( 43.630 - 43.630) [582]

...

The speculation within the maillist thread was that it was some sort of XCache configuration issue, but everything seemed to be fine for me and before the hangup MediaWiki was already succesfully able to add values to the VarCache like

mediawikitest-wiki2_:user:id:1
mediawikitest-wiki2_:resourceloader:filter:...

So i debugged my way through the code and came to the conclusion that the reason for the broken XCache support in 1.20.x are the changes made to the BagOStuff->add function:

1.19:

public function add( $key, $value, $exptime = 0 ) {

		if ( !$this->get( $key ) ) {
			$this->set( $key, $value, $exptime );

			return true;
		}

}

1.20:

public function add( $key, $value, $exptime = 0 ) {

		if ( $this->get( $key ) === false ) {
			return $this->set( $key, $value, $exptime );
		}
		return false; // key already set

}

The other keys like "mediawikitest-wiki2_:user:id:1" work fine, because MediaWiki there does not use the add function, but the set function directly instead.
So the changes now break the XCache support, because "get" basically returns the returnvalue of "xcache_get" (serialising aside, because that is a non issue here). It is basically the same function for XCache and APC and this might still work for APC because apc_fetch does infact return "false" in case the key is not found. This is not the case for XCache!
xcache_get simply does not return false in case of a not found key (its an empty string), which causes the new BagOStuff->add function to believe that the key has already been set.

Assuming the changes to the add function were made for a reason, the easiest way to fix this seems to modify the XCacheBagOStuff->get function to handle not yet set keys correctly as intended by XCache, which then would result in something like this:


class XCacheBagOStuff extends BagOStuff {

/**
 * Get a value from the XCache object cache
 *
 * @param $key String: cache key
 * @return mixed
 */
public function get( $key ) {
        if (!xcache_isset( $key )) { return false; }

        $val = xcache_get( $key );

        if ( is_string( $val ) ) {
                if ( $this->isInteger( $val ) ) {
                        $val = intval( $val );
                } else {
                        $val = unserialize( $val );
                }
        }

        return $val;
}

With this simple modification it now works fine for me.


Version: 1.20.x
Severity: major
OS: Linux
Platform: PC

Details

Reference
bz44024

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 1:18 AM
bzimport set Reference to bz44024.
bzimport added a subscriber: Unknown Object (MLST).

pierre wrote:

(bug 44024) xcache_get returns null on non-existent keys

Unfortunately I just discovered this bug report when I already debugged this issue and came to the same conclusion.

I attached a git patch for the master branch that fixes the issue. I confirmed with the XCache author that indeed null is returned in case the key does not exist. So we just test for null; saves us one call into xcache from the solution above.

Greetings,

Pierre

Attached:

overlordq wrote:

I ran into this issue today as well, commited to Gerrit.

https://gerrit.wikimedia.org/r/44754