Merge #1379

1379: Remove the N+1 queries from crates search r=jtgeibel

I've been noticing response times spiking towards 300ms when a
particular crawler hits us. Specifically one that sets `per_page=100`.
The number of queries executed to load badges is the only thing that
would dramatically change in timing based on the number of rows. This
will load the badges in 1 query per request instead of 1 query per crate
per request.

Author: bors-voyager[bot]

src/controllers/krate/search.rs

@@ -5,7 +5,7 @@ use diesel_full_text_search::*;
 use controllers::helpers::Paginate;
 use controllers::prelude::*;
 use views::EncodableCrate;
-use models::{Badge, Crate, OwnerKind, Version};
+use models::{Crate, CrateBadge, OwnerKind, Version};
 use schema::*;
 
 use models::krate::{canon_crate_name, ALL_COLUMNS};
@@ -147,50 +147,47 @@ pub fn search(req: &mut Request) -> CargoResult<Response> {
         query = query.then_order_by(crates::name.asc())
     }
 
-    // The database query returns a tuple within a tuple , with the root
+    // The database query returns a tuple within a tuple, with the root
     // tuple containing 3 items.
     let data = query
         .paginate(limit, offset)
         .load::<((Crate, bool, Option<i64>), i64)>(&*conn)?;
     let total = data.first().map(|&(_, t)| t).unwrap_or(0);
-    let crates = data.iter()
-        .map(|&((ref c, _, _), _)| c.clone())
-        .collect::<Vec<_>>();
-    let perfect_matches = data.clone()
-        .into_iter()
-        .map(|((_, b, _), _)| b)
-        .collect::<Vec<_>>();
-    let recent_downloads = data.clone()
-        .into_iter()
-        .map(|((_, _, s), _)| s.unwrap_or(0))
+    let perfect_matches = data.iter().map(|&((_, b, _), _)| b).collect::<Vec<_>>();
+    let recent_downloads = data.iter()
+        .map(|&((_, _, s), _)| s.unwrap_or(0))
         .collect::<Vec<_>>();
+    let crates = data.into_iter().map(|((c, _, _), _)| c).collect::<Vec<_>>();
 
     let versions = Version::belonging_to(&crates)
         .load::<Version>(&*conn)?
         .grouped_by(&crates)
         .into_iter()
         .map(|versions| Version::max(versions.into_iter().map(|v| v.num)));
 
+    let badges = CrateBadge::belonging_to(&crates)
+        .select((badges::crate_id, badges::all_columns))
+        .load::<CrateBadge>(&conn)?
+        .grouped_by(&crates)
+        .into_iter()
+        .map(|badges| badges.into_iter().map(|cb| cb.badge).collect());
+
     let crates = versions
         .zip(crates)
         .zip(perfect_matches)
         .zip(recent_downloads)
+        .zip(badges)
         .map(
-            |(((max_version, krate), perfect_match), recent_downloads)| {
-                // FIXME: If we add crate_id to the Badge enum we can eliminate
-                // this N+1
-                let badges = badges::table
-                    .filter(badges::crate_id.eq(krate.id))
-                    .load::<Badge>(&*conn)?;
-                Ok(krate.minimal_encodable(
+            |((((max_version, krate), perfect_match), recent_downloads), badges)| {
+                krate.minimal_encodable(
                     &max_version,
                     Some(badges),
                     perfect_match,
                     Some(recent_downloads),
-                ))
+                )
             },
         )
-        .collect::<Result<_, ::diesel::result::Error>>()?;
+        .collect();
 
     #[derive(Serialize)]
     struct R {

src/models/badge.rs

@@ -1,3 +1,4 @@
+use diesel::associations::HasTable;
 use diesel::pg::Pg;
 use diesel::prelude::*;
 use serde_json;
@@ -7,6 +8,19 @@ use models::Crate;
 use schema::badges;
 use views::EncodableBadge;
 
+/// A combination of a `Badge` and a crate ID.
+///
+/// We don't typically care about the crate ID when dealing with badges.
+/// However, when we are eager loading all badges for a group of crates, we need
+/// the crate ID to group badges by their owner.
+#[derive(Debug, Queryable, Associations)]
+#[belongs_to(Crate)]
+#[table_name = "badges"]
+pub struct CrateBadge {
+    pub crate_id: i32,
+    pub badge: Badge,
+}
+
 #[derive(Debug, PartialEq, Clone, Deserialize, Serialize)]
 #[serde(rename_all = "kebab-case", tag = "badge_type", content = "attributes")]
 pub enum Badge {
@@ -63,6 +77,15 @@ pub enum MaintenanceStatus {
     Deprecated,
 }
 
+// FIXME: Derive this in Diesel 1.3.
+impl HasTable for CrateBadge {
+    type Table = badges::table;
+
+    fn table() -> Self::Table {
+        badges::table
+    }
+}
+
 impl Queryable<badges::SqlType, Pg> for Badge {
     type Row = (i32, String, serde_json::Value);
 

src/models/mod.rs

@@ -1,4 +1,4 @@
-pub use self::badge::{Badge, MaintenanceStatus};
+pub use self::badge::{Badge, CrateBadge, MaintenanceStatus};
 pub use self::category::{Category, CrateCategory, NewCategory};
 pub use self::crate_owner_invitation::{CrateOwnerInvitation, NewCrateOwnerInvitation};
 pub use self::dependency::{Dependency, DependencyKind, ReverseDependency};