Skip to content

bugzilla
bugzilla
Commit 30084ede authored by Max Kanat-Alexander's avatar Max Kanat-Alexander
Browse files
Options

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

in a loop r=glob, a=mkanat
parent 3f40ba04
Hide whitespace changes
Inline Side-by-side
Showing with 159 additions and 54 deletions
Bugzilla.pm
View file @ 30084ede
......@@ -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)>
......
Bugzilla/Constants.pm
View file @ 30084ede
......@@ -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;
Bugzilla/Error.pm
View file @ 30084ede
......@@ -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", @_);
}
......
Bugzilla/Field.pm
View file @ 30084ede
......@@ -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 {
......
Bugzilla/Search.pm
View file @ 30084ede
......@@ -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"
......
Bugzilla/Template.pm
View file @ 30084ede
......@@ -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};
},
......
Bugzilla/User.pm
View file @ 30084ede
......@@ -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) {
......
xt/lib/Bugzilla/Test/Search.pm
View file @ 30084ede
......@@ -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;
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment