Page MenuHomePhabricator

Potential query optimization on getComments
Closed, ResolvedPublic


When I was looking at this code a couple weeks ago, I realized that running queries in a for-loop seemed a little suboptimal at first glance. Well, technically you create a new Comment object, and it immediately does two queries for each object constructed. Anyway, I decided to play around with premature optimization, and I came up with this:

Sorry, I've been too busy with work to learn how Gerrit works -- I gave up after 90 minutes or so of investigating. You're going to have to make do with github's UI, or maybe a call to git remote.

Anyway, what the branch does is reduce all of the queries needed for getComments loading down to one query. Rather than a lot of small queries (2*Ncomments + 1), we get all of the data from Comments LEFT JOIN Comments_Vote LEFT JOIN Comments_Vote (again). Mysql's EXPLAIN SELECT output leads me to think it's reasonably fast.

But back on the subject of premature optimization -- I'm not sure if it really does make it faster or not. We don't really have the data available to test, because Miraheze just installed it on our wiki farm. It's possible that small queries would be faster, depending on server configuration. Either way, I am using my branch in production (and no complaints yet). I have not tested it with other social extensions enabled, either.

So, uh, here's some code you can use, if you want. Have fun!

Event Timeline

labster created this task.May 16 2016, 9:44 AM
Restricted Application added a project: Social-Tools. · View Herald TranscriptMay 16 2016, 9:44 AM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald Transcript
SamanthaNguyen moved this task from Backlog to Comments on the Social-Tools board.Aug 1 2016, 7:41 AM
labster removed ashley as the assignee of this task.Sep 3 2016, 8:01 PM
labster added a subscriber: ashley.

No activity in over 3 months. If no one is interested in merging this code, can they please decline the task?

Can anyone take over maintainership for this extension?

Hi there and thanks for the patch!
Please accept my apologies for not getting back to you sooner -- this is by no means a trivial patch and finding people who know enough about performance matters to comment on a patch like this isn't easy either; combine all that with my other work, online and offline, and you can hopefully understand how come this patch managed to slip past my radar. :)

That being said, I'm told that the code here is definitely better than what the master version of the extension has, so I'll try to prepare a patch in gerrit soon-ish and merge it.

For speeding up the review process in the future, you'll definitely want to upload your patches into gerrit (or if you wait a few months, we might finally get rid of gerrit for good in favor of Phabricator!). There's probably some documentation on and the helpful folks on the IRC channels know something about gerrit, too, and I've also written a relatively straightforward guide to working with git and gerrit.

Change 326404 had a related patch set uploaded (by Jack Phoenix):
SQL optimization

Your guide seems pretty good, I'll give it a shot the next time I think of a patch. Actually, BlogPageClass.php is taunting me with:

	// If you can figure out how to do this without a subquery,
	// please let me know. Until that...

The answer is to use a GROUP BY clause, of course.

Of course also in your guide I see: url = ssh://YOUR WIKITECH [email protected]:29418/mediawiki/extensions/SocialProfile.git. There's always something to fix, isn't there. I guess I could just get a Brickimedia account :)

Change 326404 merged by Jack Phoenix:
SQL optimization

ashley closed this task as Resolved.Dec 14 2016, 12:04 PM
ashley claimed this task.
ashley removed a project: Patch-For-Review.