Last modified: 2014-11-03 17:42:10 UTC
Some bits: * I don't like the unbounded cache in displayGlobalPage() * Seems like brokenLink() could check 'known' earlier to avoid DB queries (the link methods are fairly hot) * displayGlobalPage() loads user prefs to check 'globaluserpage' but we don't have global preferences on * The "if ( !trim( $parsedOutput['text']['*'] ) ) {" should use a regex or something to avoid copying the HTML string in memory (nitpick) * getCentralTouched() has an unbounded cache and does not call reuseConnection() * memcached key in getRemoteParsedText() could be too long * Are we using $wgGlobalUserPageAPIUrl? If so, can it be configured to avoid API queries? Maybe a timeout should be set for MWHttpRequest? The default $wgHTTPTimeout might be a bit high. Also, what happens when it fails? I guess you get warnings and null would get cached (the cache value check uses "!== false"). Maybe the cache period could be reduced in that case and warnings avoided.
(In reply to Aaron Schulz from comment #1) > * I don't like the unbounded cache in displayGlobalPage() I set it to 100, is that too high or too low? > * Seems like brokenLink() could check 'known' earlier to avoid DB queries > (the link methods are fairly hot) Done, and I further optimized it so it doesn't end up recursively calling itself. > * displayGlobalPage() loads user prefs to check 'globaluserpage' but we > don't have global preferences on Wrapped the check in a class_exists( 'GlobalPreferences' ) block > * getCentralTouched() has an unbounded cache and does not call > reuseConnection() Set the cache limit to 100, and I think I'm properly using reuseConnection, but not totally sure. > * memcached key in getRemoteParsedText() could be too long Wrapped the key in md5() > * Are we using $wgGlobalUserPageAPIUrl? If so, can it be configured to avoid > API queries? We have to use it for the action=parse request. We can use $wgConf for the remote url, so I changed that. > Maybe a timeout should be set for MWHttpRequest? The default $wgHTTPTimeout > might be a bit high. Also, what happens when it fails? I guess you get > warnings and null would get cached (the cache value check uses "!== false"). > Maybe the cache period could be reduced in that case and warnings avoided. The default timeout in core and on the cluster is 25, so I set it to 20. Should it be lower? Also adjusted the code to gracefully fail if the API request does. Change-Id: I7440151f29bf38b6c135d8508ac1a0cf24a5677e
Change 169653 had a related patch set uploaded by Legoktm: Updates after performance review https://gerrit.wikimedia.org/r/169653
Change 169653 merged by jenkins-bot: Updates after performance review https://gerrit.wikimedia.org/r/169653
Also displayGlobalPage() checks for name validity twice and MapCacheLRU overhead probably isn't worth the avoiding the first the checks of canBeGlobal() sometimes. Maybe canBeGlobal() can just do those 3 and that can be before the cache check in displayGlobalPage().
Change 170363 had a related patch set uploaded by Legoktm: Further optimize displayGlobalPage() https://gerrit.wikimedia.org/r/170363
Change 170363 merged by jenkins-bot: Further optimize displayGlobalPage() https://gerrit.wikimedia.org/r/170363
LGTM.
