Page MenuHomePhabricator

Performance review of "Explore similar"
Closed, DeclinedPublic


We noticed that this new feature is scheduled for deployment in Q1:

A quick glance showed that there are a few areas that could use performance tracking or improvements.

This review will be an ongoing conversation, not a single report.

Event Timeline

Gilles created this task.Jul 3 2017, 5:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 3 2017, 5:45 PM
Gilles added a comment.Jul 3 2017, 5:51 PM

We tried it out and looked at API calls in particular.

The "morelike" call has smaxage and maxage parameters for caching, but the other two (categories and langlinks) don't. Is that deliberate?

All 3 API calls are independent, but the entire UI that summons them comes together when you hover a search result description. All 3 API calls seem to perform quite well, but in this current setup, every hover that triggers one will be affected by latency. Why not load the result of all 3 API calls together? Either right when the hover UI appears, after a small delay, or just when either of those 3 hover buttons is hovered.

The API backend seems to be able to combine all 3 fine:*&lllimit=500&cllimit=10&clshow=!hidden

debt added subscribers: Jdrewniak, debt.

The explore similar is currently in an A/B test and we still have more work to do before it's production ready. Thanks for the feedback and @Jdrewniak will have a look once he's back from vacation. :)

debt triaged this task as Medium priority.Jul 3 2017, 8:29 PM
debt moved this task from Incoming to UI on the Discovery-Search (Current work) board.
Krinkle added a subscriber: Krinkle.Jul 6 2017, 6:58 PM
Gilles moved this task from Inbox to Doing on the Performance-Team board.Jul 6 2017, 7:30 PM

@Gilles that API wizardry is impressive. I was unaware that API calls could be combined like that, but since they can, it seems like a good idea to me. The thinking behind making these calls separately though, was to avoid sending down data that the user might never see (in the case that they hover over 'languages' but not 'categories' for example). The cost to this approach though, like you mention, is having to wait for each response separately.

Given that we want to encourage users to 'explore' these links, I think reducing the API lag between each 'explore similar' section is desirable and is probably an overall better experience.

Indeed. The responses are gzipped and come with headers, so 3 combined together will be smaller than the sum of 3 individual requests. And the small difference in terms of actual gzipped content might so negligible it wouldn't represent that many extra TCP packets, if any. The real enemy is latency, so anything we can do to bundle small requests that happen in a small timeframe helps.

This kind of decision is always a tradeoff, but in this case I looked at the performance (not a scientific test, just comparing uncached calls on my machine) and found that the bundled call with the 3 responses together took no longer than any of these calls on its own (everything included), so it seems really a no-brainer to combine them, imho. You essentially get all 3 for the cost of 1.

Gilles added a comment.EditedJul 10 2017, 6:46 PM

So I looked out of curiosity at a random one to see how the size behaved. 3 separate calls are reported as a total transfer size of 3.8kb by Chrome. The meta API call combining those 3 is reported as a total transfer size of 1.3kb. So that clearly explains why I was experiencing the combined one being as fast as any of the 3. The overhead of the separate calls is probably due to headers and SSL for the most part, which are shared in the case of a combined API call.

Gilles claimed this task.Jul 12 2017, 7:15 PM
EBernhardson added a subscriber: EBernhardson.EditedJul 20 2017, 5:23 PM

One difficulty to combining all three is that more like is *very* expensive. So expensive that we limit the number of concurrent requests to only 50 in the backend. Before we had this limit in place (and instead combined it with the normal search limits) a request rate of around 300req/s overloaded the search cluster and increased latency of all backend search requests, even those with p50's of 10ms, into multi-second requests.

The related articles extension, which shows more like results as well, calls this millions of times an hour so we depend on varnish-level caching to prevent a flood of requets to the backind. In my opinion the more like request should ensure it looks exactly the same as the related articles api request, so that they both use the same caching. The other two requests can be combined as necessary.

@EBernhardson sounds good to me, better chances of combining Varnish hits with another feature is definitely the most important optimization

Change 366849 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/CirrusSearch@master] Combining API calls in Explore Similar widget

debt removed a project: Patch-For-Review.
debt added a comment.Sep 5 2017, 3:38 PM

Yup, we're still working on this, @Krinkle, thanks for checking.

Gilles moved this task from Doing to Radar on the Performance-Team board.Sep 12 2017, 9:01 AM
Gilles edited projects, added Performance-Team (Radar); removed Performance-Team.
debt closed this task as Declined.Sep 12 2017, 5:42 PM

It looks like we don't need this check-in anymore: as we're not combining the calls with the next test we're doing: T175647.

Gilles reopened this task as Open.Sep 13 2017, 2:17 PM

I would like to keep this open, since it sounds like you're testing drastic changes to the UX, which should warrant another pass of performance review once they're done. Plus it sounds like the new version being tested might not replace the original, depending on the outcome of the test?

Hi @Gilles, if the new test performs well, we would use that version (or something very much like it) in production, after another performance review. But, we need to test first and see if we get the percentages of views and clickthroughs that we're wanting. :)

Change 366849 abandoned by Krinkle:
Combining API calls in Explore Similar widget


debt closed this task as Declined.Sep 22 2017, 2:27 PM
debt moved this task from UI to Needs Reporting on the Discovery-Search (Current work) board.

We will not be going forward with the explore similar feature as noted here: T175647#3627249.