Skip to content

Commit

Permalink
Change the default sorting on search to something less expensive
Browse files Browse the repository at this point in the history
This changes the behavior of `/api/v1/crates` if no sort option is
provided. This does not change the behavior of the website when accessed
through a browser, as the Ember frontend always specifies a sort field.

The query run by search when sorting by recent downloads currently
accounts for nearly 40% of our total DB load. It takes ~60ms on average,
which means it's extremely high load. Our logs for traffic to this
endpoint with an explicit `sort=recent-downloads` don't match up with
this. We can fix this by making the query faster, but this has drawbacks
that were determined not to be worth it (see #1755). So if we can't
decrease execution time, the only other option is to decrease execution
count.

A significant number of crawlers hit us without specifying any sorting,
which should mean they don't care about ordering. I'm sure there's
someone out there who is relying on this behavior, but most crawlers
I've seen don't specify ordering, and none of them care about the order
they get the results in since they're crawling the whole registry
anyway.

What's important is that this lowers the execution time from ~60ms to
~12ms. And sorting on recent_downloads will grow linearly with the table
size, while this can be done on an index alone (which should be
log(log(n)) IIRC). >90% of the query is spent on getting the count,
which will be going away soon as we change pagination strategies,
bringing us to sub-millisecond execution.

I do feel strongly that we either need to make this change, or accept
the drawbacks in #1755, as this will soon become a scaling issue for us.
  • Loading branch information
sgrif committed Sep 5, 2019
1 parent 6514cdb commit 75f1b01
Showing 1 changed file with 4 additions and 7 deletions.
11 changes: 4 additions & 7 deletions src/controllers/krate/search.rs
Expand Up @@ -37,10 +37,7 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {

let conn = req.db_conn()?;
let params = req.query();
let sort = params
.get("sort")
.map(|s| &**s)
.unwrap_or("recent-downloads");
let sort = params.get("sort").map(|s| &**s);
let include_yanked = params
.get("include_yanked")
.map(|s| s == "yes")
Expand Down Expand Up @@ -156,11 +153,11 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
));
}

if sort == "downloads" {
if sort == Some("downloads") {
query = query.then_order_by(crates::downloads.desc())
} else if sort == "recent-downloads" {
} else if sort == Some("recent-downloads") {
query = query.then_order_by(recent_crate_downloads::downloads.desc().nulls_last())
} else if sort == "recent-updates" {
} else if sort == Some("recent-updates") {
query = query.order(crates::updated_at.desc());
} else {
query = query.then_order_by(crates::name.asc())
Expand Down

0 comments on commit 75f1b01

Please sign in to comment.