Bug 372795: Implement Bugzilla::Product::preload() to speed up query.cgi when there are many products

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=mkanat

3 files changed, 81 insertions, 46 deletions

diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 23a88855f..d616bb2da 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -129,52 +129,46 @@ sub new_from_list {
     my $invocant = shift;
     my $class = ref($invocant) || $invocant;
     my ($id_list) = @_;
-    my $dbh = Bugzilla->dbh;
-    my $columns = join(',', $class->DB_COLUMNS);
-    my $table   = $class->DB_TABLE;
-    my $order   = $class->LIST_ORDER;
     my $id_field = $class->ID_FIELD;
 
-    my $objects;
-    if (@$id_list) {
-        my @detainted_ids;
-        foreach my $id (@$id_list) {
-            detaint_natural($id) ||
-                ThrowCodeError('param_must_be_numeric',
-                              {function => $class . '::new_from_list'});
-            push(@detainted_ids, $id);
-        }
-        $objects = $dbh->selectall_arrayref(
-            "SELECT $columns FROM $table WHERE " 
-            . $dbh->sql_in($id_field, \@detainted_ids) 
-            . "ORDER BY $order", {Slice=>{}});
-    } else {
-        return [];
+    my @detainted_ids;
+    foreach my $id (@$id_list) {
+        detaint_natural($id) ||
+            ThrowCodeError('param_must_be_numeric',
+                          {function => $class . '::new_from_list'});
+        push(@detainted_ids, $id);
     }
-
-    foreach my $object (@$objects) {
-        bless($object, $class);
-    }
-    return $objects;
+    # We don't do $invocant->match because some classes have
+    # their own implementation of match which is not compatible
+    # with this one. However, match() still needs to have the right $invocant
+    # in order to do $class->DB_TABLE and so on.
+    return match($invocant, { $id_field => \@detainted_ids });
 }
 
 # Note: Future extensions to this could be:
-#  * Accept arrays for an IN clause
 #  * Add a MATCH_JOIN constant so that we can join against
 #    certain other tables for the WHERE criteria.
 sub match {
     my ($invocant, $criteria) = @_;
     my $class = ref($invocant) || $invocant;
     my $dbh   = Bugzilla->dbh;
-    my $id    = $class->ID_FIELD;
-    my $table = $class->DB_TABLE;
 
     return [$class->get_all] if !$criteria;
 
     my (@terms, @values);
     foreach my $field (keys %$criteria) {
         my $value = $criteria->{$field};
-        if ($value eq NOT_NULL) {
+        if (ref $value eq 'ARRAY') {
+            # IN () is invalid SQL, and if we have an empty list
+            # to match against, we're just returning an empty
+            # array anyhow.
+            return [] if !scalar @$value;
+
+            my @qmarks = ("?") x @$value;
+            push(@terms, $dbh->sql_in($field, \@qmarks));
+            push(@values, @$value);
+        }
+        elsif ($value eq NOT_NULL) {
             push(@terms, "$field IS NOT NULL");
         }
         elsif ($value eq IS_NULL) {
@@ -187,11 +181,25 @@ sub match {
     }
 
     my $where = join(' AND ', @terms);
-    my $ids   = $dbh->selectcol_arrayref(
-        "SELECT $id FROM $table WHERE $where", undef, @values)
-        || [];
+    return $class->_do_list_select($where, \@values);
+}
+
+sub _do_list_select {
+    my ($class, $where, $values) = @_;
+    my $table = $class->DB_TABLE;
+    my $cols  = join(',', $class->DB_COLUMNS);
+    my $order = $class->LIST_ORDER;
+
+    my $sql = "SELECT $cols FROM $table";
+    if (defined $where) {
+        $sql .= " WHERE $where ";
+    }
+    $sql .= " ORDER BY $order";
 
-    return $class->new_from_list($ids);
+    my $dbh = Bugzilla->dbh;
+    my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @$values);
+    bless ($_, $class) foreach @$objects;
+    return $objects
 }
 
 ###############################
@@ -349,16 +357,7 @@ sub insert_create_data {
 
 sub get_all {
     my $class = shift;
-    my $dbh = Bugzilla->dbh;
-    my $table = $class->DB_TABLE;
-    my $order = $class->LIST_ORDER;
-    my $id_field = $class->ID_FIELD;
-
-    my $ids = $dbh->selectcol_arrayref(qq{
-        SELECT $id_field FROM $table ORDER BY $order});
-
-    my $objects = $class->new_from_list($ids);
-    return @$objects;
+    return @{$class->_do_list_select()};
 }
 
 ###############################
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 45c489973..edc621f67 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -51,6 +51,32 @@ use constant DB_COLUMNS => qw(
    products.defaultmilestone
 );
 
+###############################
+####     Constructors     #####
+###############################
+
+# This is considerably faster than calling new_from_list three times
+# for each product in the list, particularly with hundreds or thousands
+# of products.
+sub preload {
+    my ($products) = @_;
+    my %prods = map { $_->id => $_ } @$products;
+    my @prod_ids = keys %prods;
+    return unless @prod_ids;
+
+    my $dbh = Bugzilla->dbh;
+    foreach my $field (qw(component version milestone)) {
+        my $classname = "Bugzilla::" . ucfirst($field);
+        my $objects = $classname->match({ product_id => \@prod_ids });
+
+        # Now populate the products with this set of objects.
+        foreach my $obj (@$objects) {
+            my $product_id = $obj->product_id;
+            $prods{$product_id}->{"${field}s"} ||= [];
+            push(@{$prods{$product_id}->{"${field}s"}}, $obj);
+        }
+    }
+}
 
 ###############################
 ####       Methods         ####
@@ -300,7 +326,7 @@ below.
 
 =over
 
-=item C<components()>
+=item C<components>
 
  Description: Returns an array of component objects belonging to
               the product.
@@ -319,7 +345,7 @@ below.
  Returns:     A hash with group id as key and hash containing 
               a Bugzilla::Group object and the properties of group
               relative to the product.
-              
+
 =item C<groups_mandatory_for>
 
 =over
@@ -356,7 +382,7 @@ groups, in this product.
 
 =back
 
-=item C<versions()>
+=item C<versions>
 
  Description: Returns all valid versions for that product.
 
@@ -364,7 +390,7 @@ groups, in this product.
 
  Returns:     An array of Bugzilla::Version objects.
 
-=item C<milestones()>
+=item C<milestones>
 
  Description: Returns all valid milestones for that product.
 
@@ -415,6 +441,15 @@ groups, in this product.
 
 =over
 
+=item C<preload>
+
+When passed an arrayref of C<Bugzilla::Product> objects, preloads their
+L</milestones>, L</components>, and L</versions>, which is much faster
+than calling those accessors on every item in the array individually.
+
+This function is not exported, so must be called like 
+C<Bugzilla::Product::preload($products)>.
+
 =item C<check_product($product_name)>
 
  Description: Checks if the product name was passed in and if is a valid
diff --git a/query.cgi b/query.cgi
index dbdf69be9..5998c31ab 100755
--- a/query.cgi
+++ b/query.cgi
@@ -189,6 +189,7 @@ if (!scalar(@{$default{'chfieldto'}}) || $default{'chfieldto'}->[0] eq "") {
 # don't have access to. Remove them from the list.
 my @selectable_products = sort {lc($a->name) cmp lc($b->name)} 
                                @{$user->get_selectable_products};
+Bugzilla::Product::preload(\@selectable_products);
 
 # Create the component, version and milestone lists.
 my %components;