summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMax Kanat-Alexander <mkanat@bugzilla.org>2010-07-09 04:00:31 +0200
committerMax Kanat-Alexander <mkanat@bugzilla.org>2010-07-09 04:00:31 +0200
commit30084ede70b1f17b620f5bb5d38ccabb3321f5df (patch)
treee55efb88fa4db408220e73d0279702ed35af7403
parent3f40ba04a7bdea2f3f84202006cc55054d647afb (diff)
downloadbugzilla-30084ede70b1f17b620f5bb5d38ccabb3321f5df.tar.gz
bugzilla-30084ede70b1f17b620f5bb5d38ccabb3321f5df.tar.xz
Bug 576670: Optimize Search.pm's "init" method for being called many times
in a loop r=glob, a=mkanat
-rw-r--r--Bugzilla.pm63
-rw-r--r--Bugzilla/Constants.pm15
-rw-r--r--Bugzilla/Error.pm16
-rw-r--r--Bugzilla/Field.pm2
-rw-r--r--Bugzilla/Search.pm61
-rw-r--r--Bugzilla/Template.pm4
-rw-r--r--Bugzilla/User.pm50
-rw-r--r--xt/lib/Bugzilla/Test/Search.pm2
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;
}