Page MenuHomePhabricator

Performance review of GlobalUserPage extension
Closed, ResolvedPublic

Description


Version: unspecified
Severity: normal

Details

Reference
bz70585

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:43 AM
bzimport added a project: GlobalUserPage.
bzimport set Reference to bz70585.
Legoktm created this task.Sep 8 2014, 9:43 PM
aaron added a comment.Oct 28 2014, 7:03 PM

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

aaron added a comment.Oct 30 2014, 9:10 PM

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