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

gerritadmin wrote:

Change 169653 had a related patch set uploaded by Legoktm:
Updates after performance review

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

gerritadmin wrote:

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().

gerritadmin wrote:

Change 170363 had a related patch set uploaded by Legoktm:
Further optimize displayGlobalPage()

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

gerritadmin wrote:

Change 170363 merged by jenkins-bot:
Further optimize displayGlobalPage()

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

LGTM.

LGTM.