Last modified: 2014-11-03 17:42:10 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 70585 - Performance review of GlobalUserPage extension
Performance review of GlobalUserPage extension
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
GlobalUserPage (Other open bugs)
unspecified
All All
: Unprioritized normal (vote)
: ---
Assigned To: Aaron Schulz
:
Depends on:
Blocks: 70576
  Show dependency treegraph
 
Reported: 2014-09-08 21:43 UTC by Kunal Mehta (Legoktm)
Modified: 2014-11-03 17:42 UTC (History)
6 users (show)

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


Attachments

Description Kunal Mehta (Legoktm) 2014-09-08 21:43:42 UTC

    
Comment 1 Aaron Schulz 2014-10-28 19:03:31 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.
Comment 2 Kunal Mehta (Legoktm) 2014-10-29 04:31:21 UTC
(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
Comment 3 Gerrit Notification Bot 2014-10-29 04:49:58 UTC
Change 169653 had a related patch set uploaded by Legoktm:
Updates after performance review

https://gerrit.wikimedia.org/r/169653
Comment 4 Gerrit Notification Bot 2014-10-30 20:21:44 UTC
Change 169653 merged by jenkins-bot:
Updates after performance review

https://gerrit.wikimedia.org/r/169653
Comment 5 Aaron Schulz 2014-10-30 21:10:21 UTC
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().
Comment 6 Gerrit Notification Bot 2014-10-31 17:59:57 UTC
Change 170363 had a related patch set uploaded by Legoktm:
Further optimize displayGlobalPage()

https://gerrit.wikimedia.org/r/170363
Comment 7 Gerrit Notification Bot 2014-10-31 22:23:22 UTC
Change 170363 merged by jenkins-bot:
Further optimize displayGlobalPage()

https://gerrit.wikimedia.org/r/170363
Comment 8 Aaron Schulz 2014-11-03 17:42:10 UTC
LGTM.
Comment 9 Aaron Schulz 2014-11-03 17:42:10 UTC
LGTM.

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


Navigation
Links