Bug 576670: Optimize Search.pm's "init" method for being called many times

in a loop
r=glob, a=mkanat

8 files changed, 159 insertions, 54 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index 33df05efb..6ecbc27db 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -523,6 +523,45 @@ sub switch_to_main_db {
     return $class->dbh_main;
 }
 
+sub fields {
+    my ($class, $criteria) = @_;
+    $criteria ||= {};
+    my $cache = $class->request_cache;
+
+    # We create an advanced cache for fields by type, so that we
+    # can avoid going back to the database for every fields() call.
+    # (And most of our fields() calls are for getting fields by type.)
+    #
+    # We also cache fields by name, because calling $field->name a few
+    # million times can be slow in calling code, but if we just do it
+    # once here, that makes things a lot faster for callers.
+    if (!defined $cache->{fields}) {
+        my @all_fields = Bugzilla::Field->get_all;
+        my (%by_name, %by_type);
+        foreach my $field (@all_fields) {
+            my $name = $field->name;
+            $by_type{$field->type}->{$name} = $field;
+            $by_name{$name} = $field;
+        }
+        $cache->{fields} = { by_type => \%by_type, by_name => \%by_name };
+    }
+
+    my $fields = $cache->{fields};
+    my %requested;
+    if (my $types = $criteria->{type}) {
+        $types = ref($types) ? $types : [$types];
+        %requested = map { %{ $fields->{by_type}->{$_} || {} } } @$types;
+    }
+    else {
+        %requested = %{ $fields->{by_name} };
+    }
+    
+    my $do_by_name = $criteria->{by_name};
+
+    return $do_by_name ? \%requested : [values %requested];
+}
+
+# DEPRECATED. Use fields() instead.
 sub get_fields {
     my $class = shift;
     my $criteria = shift;
@@ -768,6 +807,30 @@ Essentially, causes calls to C<Bugzilla-E<gt>user> to return C<undef>. This has
 effect of logging out a user for the current request only; cookies and
 database sessions are left intact.
 
+=item C<fields>
+
+This is the standard way to get arrays or hashes of L<Bugzilla::Field>
+objects when you need them. It takes the following named arguments
+in a hashref:
+
+=over
+
+=item C<by_name>
+
+If false (or not specified), this method will return an arrayref of
+the requested fields. The order of the returned fields is random.
+
+If true, this method will return a hashref of fields, where the keys
+are field names and the valules are L<Bugzilla::Field> objects.
+
+=item C<type>
+
+Either a single C<FIELD_TYPE_*> constant or an arrayref of them. If specified,
+the returned fields will be limited to the types in the list. If you don't
+specify this argument, all fields will be returned.
+
+=back
+
 =item C<error_mode>
 
 Call either C<Bugzilla->error_mode(Bugzilla::Constants::ERROR_MODE_DIE)>
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index d58fe9388..2285f89b0 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -35,6 +35,7 @@ use base qw(Exporter);
 
 # For bz_locations
 use File::Basename;
+use Memoize;
 
 @Bugzilla::Constants::EXPORT = qw(
     BUGZILLA_VERSION
@@ -404,11 +405,11 @@ use constant EMPTY_DATETIME_REGEX => qr/^[0\-:\sA-Za-z]+$/;
 
 # See the POD for Bugzilla::Field/is_abnormal to see why these are listed
 # here.
-use constant ABNORMAL_SELECTS => qw(
-    classification
-    product
-    component
-);
+use constant ABNORMAL_SELECTS => {
+    classification => 1,
+    component      => 1,
+    product        => 1,
+};
 
 # The fields from fielddefs that are blocked from non-timetracking users.
 # work_time is sometimes called actual_time.
@@ -619,4 +620,8 @@ sub bz_locations {
     };
 }
 
+# This makes us not re-compute all the bz_locations data every time it's
+# called.
+BEGIN { memoize('bz_locations') };
+
 1;
diff --git a/Bugzilla/Error.pm b/Bugzilla/Error.pm
index 649fdd486..395cc0dc9 100644
--- a/Bugzilla/Error.pm
+++ b/Bugzilla/Error.pm
@@ -53,12 +53,6 @@ sub _throw_error {
     $vars ||= {};
 
     $vars->{error} = $error;
-    # Don't show function arguments, in case they contain confidential data.
-    local $Carp::MaxArgNums = -1;
-    # Don't show the error as coming from Bugzilla::Error, show it as coming
-    # from the caller.
-    local $Carp::CarpInternal{'Bugzilla::Error'} = 1; 
-    $vars->{traceback} = Carp::longmess();
 
     # Make sure any transaction is rolled back (if supported).
     # If we are within an eval(), do not roll back transactions as we are
@@ -159,6 +153,16 @@ sub ThrowUserError {
 }
 
 sub ThrowCodeError {
+    my (undef, $vars) = @_;
+
+    # Don't show function arguments, in case they contain
+    # confidential data.
+    local $Carp::MaxArgNums = -1;
+    # Don't show the error as coming from Bugzilla::Error, show it
+    # as coming from the caller.
+    local $Carp::CarpInternal{'Bugzilla::Error'} = 1;
+    $vars->{traceback} = Carp::longmess();
+
     _throw_error("global/code-error.html.tmpl", @_);
 }
 
diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
index 26025015a..c0e88fc37 100644
--- a/Bugzilla/Field.pm
+++ b/Bugzilla/Field.pm
@@ -544,7 +544,7 @@ This method returns C<1> if the field is "abnormal", C<0> otherwise.
 
 sub is_abnormal {
     my $self = shift;
-    return grep($_ eq $self->name, ABNORMAL_SELECTS) ? 1 : 0;
+    return ABNORMAL_SELECTS->{$self->name} ? 1 : 0;
 }
 
 sub legal_values {
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 3653cff67..63e458655 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -461,13 +461,11 @@ sub init {
     my %special_order      = %{SPECIAL_ORDER()};
     my %special_order_join = %{SPECIAL_ORDER_JOIN()};
 
-    my @select_fields = 
-        Bugzilla->get_fields({ type => FIELD_TYPE_SINGLE_SELECT });
+    my $select_fields = Bugzilla->fields({ type => FIELD_TYPE_SINGLE_SELECT });
     
-    my @multi_select_fields = Bugzilla->get_fields({
-        type     => [FIELD_TYPE_MULTI_SELECT, FIELD_TYPE_BUG_URLS],
-        obsolete => 0 });
-    foreach my $field (@select_fields) {
+    my $multi_select_fields = Bugzilla->fields({
+        type => [FIELD_TYPE_MULTI_SELECT, FIELD_TYPE_BUG_URLS]});
+    foreach my $field (@$select_fields) {
         next if $field->is_abnormal;
         my $name = $field->name;
         $special_order{$name} = [ "$name.sortkey", "$name.value" ],
@@ -522,7 +520,7 @@ sub init {
         push(@supptables, "LEFT JOIN longdescs AS ldtime " .
                           "ON ldtime.bug_id = bugs.bug_id");
     }
-    foreach my $field (@multi_select_fields) {
+    foreach my $field (@$multi_select_fields) {
         my $field_name = $field->name;
         next if !grep($_ eq $field_name, @fields);
         push(@supptables, "LEFT JOIN bug_$field_name AS map_bug_$field_name"
@@ -594,15 +592,18 @@ sub init {
     
     # All fields that don't have a . in their name should be specifyable
     # in the URL directly.
-    my @legal_fields = grep { $_->name !~ /\./ } Bugzilla->get_fields;
+    my $legal_fields = Bugzilla->fields({ by_name => 1 });
     if (!$user->is_timetracker) {
-        foreach my $field (TIMETRACKING_FIELDS) {
-            @legal_fields = grep { $_->name ne $field } @legal_fields;
+        foreach my $name (TIMETRACKING_FIELDS) {
+            delete $legal_fields->{$name};
         }
     }
+    foreach my $name (keys %$legal_fields) {
+        delete $legal_fields->{$name} if $name =~ /\./;
+    }
 
     foreach my $field ($params->param()) {
-        if (grep { $_->name eq $field } @legal_fields) {
+        if ($legal_fields->{$field}) {
             my $type = $params->param("${field}_type");
             if (!$type) {
                 if ($field eq 'keywords') {
@@ -940,12 +941,11 @@ sub init {
 # $suppstring = String which is pasted into query containing all table names
 
     # get a list of field names to verify the user-submitted chart fields against
-    my %chartfields = @{$dbh->selectcol_arrayref(
-        q{SELECT name, id FROM fielddefs}, { Columns=>[1,2] })};
+    my $chart_fields = Bugzilla->fields({ by_name => 1 });
 
     if (!$user->is_timetracker) {
         foreach my $tt_field (TIMETRACKING_FIELDS) {
-            delete $chartfields{$tt_field};
+            delete $chart_fields->{$tt_field};
         }
     }
 
@@ -976,12 +976,12 @@ sub init {
 
                 # chart -1 is generated by other code above, not from the user-
                 # submitted form, so we'll blindly accept any values in chart -1
-                if (!$chartfields{$field} and $chart != -1) {
+                if (!$chart_fields->{$field} and $chart != -1) {
                     ThrowCodeError("invalid_field_name", { field => $field });
                 }
 
                 # This is either from the internal chart (in which case we
-                # already know about it), or it was in %chartfields, so it is
+                # already know about it), or it was in $chart_fields, so it is
                 # a valid field name, which means that it's ok.
                 trick_taint($field);
                 my $quoted = $dbh->quote($value);
@@ -996,13 +996,13 @@ sub init {
                     operator   => $operator,
                     value      => $value,
                     quoted     => $quoted,
-                    multi_fields => \@multi_select_fields,
+                    multi_fields => $multi_select_fields,
                     joins        => \@supptables,
                     where        => \@wherepart,
                     having       => \@having,
                     group_by     => \@groupby,
                     fields       => \@fields,
-                    chart_fields => \%chartfields,
+                    chart_fields => $chart_fields,
                 );
                 # This should add a "term" selement to %search_args.
                 $self->do_search_function(\%search_args);
@@ -2358,13 +2358,16 @@ sub _nowords {
 
 sub _changedbefore_changedafter {
     my ($self, $args) = @_;
-    my ($chart_id, $joins, $field, $operator, $value) =
-        @$args{qw(chart_id joins field operator value)};
+    my ($chart_id, $joins, $field, $operator, $value, $chart_fields) =
+        @$args{qw(chart_id joins field operator value chart_fields)};
     my $dbh = Bugzilla->dbh;
     
     my $sql_operator = ($operator =~ /before/) ? '<' : '>';
     my $table = "act_$chart_id";
-    my $field_id = get_field_id($field);
+    my $field_object = $chart_fields->{$field}
+        || ThrowCodeError("invalid_field_name", { field => $field });
+    my $field_id = $field_object->id;
+    
     my $sql_date = $dbh->quote(SqlifyDate($value));
     push(@$joins,
         "LEFT JOIN bugs_activity AS $table"
@@ -2376,12 +2379,14 @@ sub _changedbefore_changedafter {
 
 sub _changedfrom_changedto {
     my ($self, $args) = @_;
-    my ($chart_id, $joins, $field, $operator, $quoted) =
-        @$args{qw(chart_id joins field operator quoted)};
+    my ($chart_id, $joins, $field, $operator, $quoted, $chart_fields) =
+        @$args{qw(chart_id joins field operator quoted chart_fields)};
     
     my $column = ($operator =~ /from/) ? 'removed' : 'added';
     my $table = "act_$chart_id";
-    my $field_id = get_field_id($field);
+    my $field_object = $chart_fields->{$field}
+        || ThrowCodeError("invalid_field_name", { field => $field });
+    my $field_id = $field_object->id;
     push(@$joins,
         "LEFT JOIN bugs_activity AS $table"
         .     " ON $table.bug_id = bugs.bug_id"
@@ -2392,11 +2397,13 @@ sub _changedfrom_changedto {
 
 sub _changedby {
     my ($self, $args) = @_;
-    my ($chart_id, $joins, $field, $operator, $value) =
-        @$args{qw(chart_id joins field operator value)};
+    my ($chart_id, $joins, $field, $operator, $value, $chart_fields) =
+        @$args{qw(chart_id joins field operator value chart_fields)};
     
     my $table = "act_$chart_id";
-    my $field_id = get_field_id($field);
+    my $field_object = $chart_fields->{$field}
+        || ThrowCodeError("invalid_field_name", { field => $field });
+    my $field_id = $field_object->id;
     my $user_id  = login_to_id($value, THROW_ERROR);
     push(@$joins,
         "LEFT JOIN bugs_activity AS $table"
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index 4bd3d2d77..2c2d402a3 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -765,8 +765,8 @@ sub create {
             # A way for all templates to get at Field data, cached.
             'bug_fields' => sub {
                 my $cache = Bugzilla->request_cache;
-                $cache->{template_bug_fields} ||= 
-                    { map { $_->name => $_ } Bugzilla->get_fields() };
+                $cache->{template_bug_fields} ||=
+                    Bugzilla->fields({ by_name => 1 });
                 return $cache->{template_bug_fields};
             },
 
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 78e92eca0..09af233ec 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -603,18 +603,28 @@ sub groups {
     return $self->{groups};
 }
 
+# It turns out that calling ->id on objects a few hundred thousand
+# times is pretty slow. (It showed up as a significant time contributor
+# when profiling xt/search.t.) So we cache the group ids separately from
+# groups for functions that need the group ids.
+sub _group_ids {
+    my ($self) = @_;
+    $self->{group_ids} ||= [map { $_->id } @{ $self->groups }];
+    return $self->{group_ids};
+}
+
 sub groups_as_string {
     my $self = shift;
-    my @ids = map { $_->id } @{ $self->groups };
-    return scalar(@ids) ? join(',', @ids) : '-1';
+    my $ids = $self->_group_ids;
+    return scalar(@$ids) ? join(',', @$ids) : '-1';
 }
 
 sub groups_in_sql {
     my ($self, $field) = @_;
     $field ||= 'group_id';
-    my @ids = map { $_->id } @{ $self->groups };
-    @ids = (-1) if !scalar @ids;
-    return Bugzilla->dbh->sql_in($field, \@ids);
+    my $ids = $self->_group_ids;
+    $ids = [-1] if !scalar @$ids;
+    return Bugzilla->dbh->sql_in($field, $ids);
 }
 
 sub bless_groups {
@@ -1096,7 +1106,7 @@ sub queryshare_groups {
             }
         }
         else {
-            @queryshare_groups = map { $_->id } @{ $self->groups };
+            @queryshare_groups = @{ $self->_group_ids };
         }
     }
 
@@ -1848,15 +1858,31 @@ sub is_available_username {
     return 1;
 }
 
+# This is used in a few performance-critical areas where we don't want to
+# do check() and pull all the user data from the database.
 sub login_to_id {
     my ($login, $throw_error) = @_;
     my $dbh = Bugzilla->dbh;
-    # No need to validate $login -- it will be used by the following SELECT
-    # statement only, so it's safe to simply trick_taint.
-    trick_taint($login);
-    my $user_id = $dbh->selectrow_array("SELECT userid FROM profiles WHERE " .
-                                        $dbh->sql_istrcmp('login_name', '?'),
-                                        undef, $login);
+    my $cache = Bugzilla->request_cache->{user_login_to_id} ||= {};
+
+    # We cache lookups because this function showed up as taking up a 
+    # significant amount of time in profiles of xt/search.t. However,
+    # for users that don't exist, we re-do the check every time, because
+    # otherwise we break is_available_username.
+    my $user_id;
+    if (defined $cache->{$login}) {
+        $user_id = $cache->{$login};
+    }
+    else {
+        # No need to validate $login -- it will be used by the following SELECT
+        # statement only, so it's safe to simply trick_taint.
+        trick_taint($login);
+        $user_id = $dbh->selectrow_array(
+            "SELECT userid FROM profiles 
+              WHERE " . $dbh->sql_istrcmp('login_name', '?'), undef, $login);
+        $cache->{$login} = $user_id;
+    }
+
     if ($user_id) {
         return $user_id;
     } elsif ($throw_error) {
diff --git a/xt/lib/Bugzilla/Test/Search.pm b/xt/lib/Bugzilla/Test/Search.pm
index af595e373..87df927ca 100644
--- a/xt/lib/Bugzilla/Test/Search.pm
+++ b/xt/lib/Bugzilla/Test/Search.pm
@@ -147,7 +147,7 @@ sub all_fields {
     my $self = shift;
     if (not $self->{all_fields}) {
         $self->_create_custom_fields();
-        my @fields = Bugzilla->get_fields;
+        my @fields = @{ Bugzilla->fields };
         @fields = sort { $a->name cmp $b->name } @fields;
         $self->{all_fields} = \@fields;
     }