Open
Bug 1351895
(bmo-slow)
Opened 8 years ago
Updated 6 years ago
A large collection of slow things
Categories
(bugzilla.mozilla.org :: General, task, P2)
Tracking
()
NEW
People
(Reporter: dylan, Unassigned)
References
(Depends on 2 open bugs)
Details
Attachments
(6 files, 4 obsolete files)
3.63 KB,
patch
|
glob
:
review-
|
Details | Diff | Splinter Review |
5.24 KB,
patch
|
glob
:
review-
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
glob
:
review-
|
Details | Diff | Splinter Review |
6.21 KB,
patch
|
glob
:
review-
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
Details | Diff | Splinter Review | |
13.48 KB,
patch
|
glob
:
review-
|
Details | Diff | Splinter Review |
There are a lot of changes I want to make, and I may well forget them (as I have done this sort of work before). As a precaution, here are some experiments:
All of these benchmarks were conducted using Devel::NYTProf on my dev machine, running under mod_perl. ab (apache bench) was run with concurrency 2 (which matches the number of workers configure for this apache) on show_bug.cgi, with show_bug being configured for unauth'd users to get the new modal page.
Each of the changes below was test in isolation of the other changes.
Bugzilla/Util.pm:
1)
trick_taint() is slower than Taint::Util::untaint():
# spent 333ms (293+40.1) within Bugzilla::Util::trick_taint which was called 91861 times, avg 4µs/call:
# spent 26.0ms within Taint::Util::untaint which was called 91861 times, avg 283ns/call:
There is no trade-off, this is simply better. Taint::Util::untaint() directly toggles the taint flag.
2)
html_quote() is slower than HTML::Escape::escape_html():
# spent 1.19s (886ms+302ms) within Bugzilla::Util::html_quote which was called 183062 times, avg 6µs/call:
# spent 136ms within HTML::Escape::escape_html which was called 183062 times, avg 743ns/call:
For this way trade away from correcting bi-directional text and obscuring @ symbols.
extensions/TrackingFlags/lib/Flag.pm:
1)
It's better to cache in the request the list of sorted flags.
# spent 6.83s (5.56+1.27) within Bugzilla::Extension::TrackingFlags::Flag::CORE:sort which was called 785 times, avg 8.70ms/call:
# spent 3.22s (2.62+597ms) within Bugzilla::Extension::TrackingFlags::Flag::CORE:sort which was called 628 times, avg 5.12ms/call:
2)
get_all() could be cached in the config memcached cache (even for just non-logged-in users)
# spent 18.7s (210ms+18.5) within Bugzilla::Extension::TrackingFlags::Flag::get_all which was called 314 times, avg 59.5ms/call:
# spent 605ms (15.2+589) within Bugzilla::Extension::TrackingFlags::Flag::get_all which was called 314 times, avg 1.93ms/call:
Bugzilla/Template.pm:
1)
unsetting the global die handler before processing templates is very good idea.
# spent 67.2s making 157 calls to Bugzilla::Template::process, avg 428ms/call
# spent 53.5s making 158 calls to Bugzilla::Template::process, avg 339ms/call
Reporter | ||
Comment 1•8 years ago
|
||
Nothing here is particularly actionable here, so in second thought this bug can be open.
Group: bmo-infra
Reporter | ||
Comment 2•8 years ago
|
||
I think we've wanted this for a while -- it's a bit limited but it does reduce the number of SELECTS.
Attachment #8853618 -
Flags: review?(glob)
Reporter | ||
Comment 3•8 years ago
|
||
Attachment #8853619 -
Flags: review?(glob)
Reporter | ||
Comment 4•8 years ago
|
||
As far as I can tell, this is the right use of the config cache. I think it gets cleared when custom fields are added. I wonder if we can make stronger gurantees if we remove the ability to add custom fields at runtime?
Attachment #8853620 -
Flags: review?(glob)
Reporter | ||
Comment 5•8 years ago
|
||
This one is a pretty obvious in hindsight.
Attachment #8853621 -
Flags: review?(glob)
Reporter | ||
Comment 6•8 years ago
|
||
So fun thing -- using an iterator style here causes a lot more actual SELECTs to flow to the server. I'm sure that was known in the past as using fetchrow_hashref() has always been advised against for Bugzilla.
In this same area we could benefit from some memcaching too.
Attachment #8853622 -
Flags: review?(glob)
Reporter | ||
Comment 7•8 years ago
|
||
For now we just use this as trick_taint, but we can later replace for loops that use trick_taint, e.g.:
trick_taint($_) for @arr == untaint(@arr);
Attachment #8853624 -
Flags: review?(glob)
Reporter | ||
Comment 8•8 years ago
|
||
The only question here is: all or some?
For now I've just replaced id/name for the classes that get called the most.
It results in a dramatic performance improvement, and is more pronounced on larger bugs.
Attachment #8853625 -
Flags: review?(glob)
Reporter | ||
Comment 9•8 years ago
|
||
The only question here is if not obscuring the '@' and moving the Bi-Di handling (which.. this is not the right place to handle it!) is the right move or not.
Adding Bi-Di only costs a few milliseconds. This is f? because it obviously would need testing with Bi-Di text.
Attachment #8853626 -
Flags: feedback?(glob)
Reporter | ||
Comment 10•8 years ago
|
||
I rewrote a template in perl... using [% RAWPERL %].
The template was very expensive, it's now not expensive at all.
Attachment #8853627 -
Flags: review?(glob)
Reporter | ||
Comment 11•8 years ago
|
||
This is not the sort of optimization that is... very nice looking, but the performance is difficult to ignore.
Attachment #8853628 -
Flags: review?(glob)
Reporter | ||
Comment 12•8 years ago
|
||
Okay, so there's 13 patches here (and one on bug 1352264). What does all this mean, bigger picture?
Before these changes, benchmarks look like this:
https://people-mozilla.org/~dhardison/1351895/nytprof-slowest/index-subs-excl.html
Afterwards, like this:
https://people-mozilla.org/~dhardison/1351895/nytprof-fastest/index-subs-excl.html
On my dev machine with apache bench (and with profiling turned off) the difference is between 10 requests per second and 1 request per second[1]
[1] this is quite variable.
Comment 13•8 years ago
|
||
Comment on attachment 8853618 [details] [diff] [review]
new_from_where.patch
Review of attachment 8853618 [details] [diff] [review]:
-----------------------------------------------------------------
::: Bugzilla/Object.pm
@@ +419,5 @@
> if (!$objects) {
> my $sql = "SELECT $cols FROM $table";
> if (defined $where) {
> + if (ref $where && ref($where) eq 'SCALAR') {
> + $sql .= " $$where";
this feels clumsy.
a new param to trigger different behaviour instead of overloading $where would be cleaner.
::: Bugzilla/User.pm
@@ +1464,2 @@
>
> + $self->{selectable_products} = Bugzilla::Product->new_from_where(\$query);
this param is out of place - it isn't a "where" clause, and it's weird to pass in a scalar ref to just change behaviour.
perhaps you need two new_from methods, one for _where, and another for _joins_and_where
Attachment #8853618 -
Flags: review?(glob) → review-
Comment 14•8 years ago
|
||
Comment on attachment 8853620 [details] [diff] [review]
active_custom_fields.patch
Review of attachment 8853620 [details] [diff] [review]:
-----------------------------------------------------------------
> TODO: make sure this is cleared out.
action or remove TODO item
Attachment #8853620 -
Flags: review?(glob) → review-
Comment 15•8 years ago
|
||
Comment on attachment 8853621 [details] [diff] [review]
field_obj_cache.patch
Review of attachment 8853621 [details] [diff] [review]:
-----------------------------------------------------------------
r=glob
Attachment #8853621 -
Flags: review?(glob) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8853622 [details] [diff] [review]
fetchall.patch
Review of attachment 8853622 [details] [diff] [review]:
-----------------------------------------------------------------
r=glob
Attachment #8853622 -
Flags: review?(glob) → review+
Comment 17•8 years ago
|
||
Comment on attachment 8853624 [details] [diff] [review]
trick_taint.patch
Review of attachment 8853624 [details] [diff] [review]:
-----------------------------------------------------------------
::: Bugzilla/Util.pm
@@ +47,2 @@
>
> +*trick_taint = \&Taint::Util::untaint;
trick_taint returns a bool, while untaint always returns undef.
there's places in bugzilla that rely on trick_taint's result.
eg. https://github.com/mozilla-bteam/bmo/blob/e655f5133b4dd5c5d6f253069767db75c64eb349/report.cgi#L104
although this looks incorrect: https://github.com/mozilla-bteam/bmo/blob/e655f5133b4dd5c5d6f253069767db75c64eb349/extensions/ProductDashboard/lib/Queries.pm#L386
Attachment #8853624 -
Flags: review?(glob) → review-
Comment 18•8 years ago
|
||
Comment on attachment 8853625 [details] [diff] [review]
class_xs_accessor.patch
Review of attachment 8853625 [details] [diff] [review]:
-----------------------------------------------------------------
> The only question here is: all or some? For now I've just replaced id/name for the classes that get called the most.
this doesn't belong in a commit description.
::: Bugzilla/User.pm
@@ +89,5 @@
>
> +use Class::XSAccessor (
> + getters => {
> + id => ID_FIELD,
> + }
this should be the same as the other classes (id and name, using __PACKAGE__)
::: Makefile.PL
@@ +63,4 @@
> 'version' => '0.87',
> 'Taint::Util' => 0,
> 'HTML::Escape' => 0,
> + 'Class::XSAccessor' => 0,
you need 1.05 to support the |getters => [ list ]| syntax
::: extensions/TrackingFlags/lib/Flag/Value.pm
@@ +112,5 @@
> ###############################
> #### Accessors ####
> ###############################
> +use Class::XSAccessor (
> + getters => [qw[ tracking_flag_id setter_group_id value sortkey is_active comment]],
nit: missing space before ]
Attachment #8853625 -
Flags: review?(glob) → review-
Comment 19•8 years ago
|
||
Comment on attachment 8853626 [details] [diff] [review]
html_quote.patch
Review of attachment 8853626 [details] [diff] [review]:
-----------------------------------------------------------------
15 years ago s/\@/@/ may have done something, but there's no way it provides any form of protection today. happy to see it gone in favour of performance.
stripping bi-di markers should remain however.
not sure about stripping control chars; you may have to look at our data to see what impact that would have.
Attachment #8853626 -
Flags: feedback?(glob)
Comment 20•8 years ago
|
||
Comment on attachment 8853627 [details] [diff] [review]
field-help.patch
Review of attachment 8853627 [details] [diff] [review]:
-----------------------------------------------------------------
::: extensions/BugModal/template/en/default/bug_modal/field.html.tmpl
@@ +38,4 @@
>
> [%
> IF field_type.defined && !field;
> +ELSE;
this change appears unrelated.
it also breaks indentation for the whole file.
please revert.
::: template/en/default/bug/field-help.none.tmpl
@@ +230,5 @@
> +foreach my $help_field (keys %$bug_fields) {
> + if (!defined $vars->{help_html}{$help_field}) {
> + my $field_type = $bug_fields->{$help_field}{type};
> + my $field_type_desc = Bugzilla::Util::html_quote($field_types->{$field_type});
> + $vars->{help_html}{$help_field} =
indentation in this file is a mix of 2 and 4 spaces.
may as well strip all trailing whitespace while you're touching these lines.
Attachment #8853627 -
Flags: review?(glob) → review-
Comment 21•8 years ago
|
||
Comment on attachment 8853628 [details] [diff] [review]
field-descs.patch
Review of attachment 8853628 [details] [diff] [review]:
-----------------------------------------------------------------
::: template/en/default/global/field-descs.none.tmpl
@@ +69,5 @@
> + Bugzilla::Constants::FIELD_TYPE_TEXTAREA() => "Large Text Box",
> + Bugzilla::Constants::FIELD_TYPE_DATETIME() => "Date/Time",
> + Bugzilla::Constants::FIELD_TYPE_DATE() => "Date",
> + Bugzilla::Constants::FIELD_TYPE_BUG_ID() => "$terms->{Bug} ID",
> + Bugzilla::Constants::FIELD_TYPE_EXTENSION() => "Extension",
align all the =>'s
@@ +166,5 @@
> + foreach my $bz_field ( values %$bug_fields ) {
> + my $name = $bz_field->name;
> + unless ( defined $vars->{field_descs}{$name} ) {
> + $vars->{field_descs}{$name} = $bz_field->description;
> + }
avoiding the temp var should be faster:
$vars->{field_descs}{$bz_field->name} //= $bz_field->description;
Attachment #8853628 -
Flags: review?(glob) → review+
Reporter | ||
Updated•8 years ago
|
Priority: P1 → P2
Comment 23•8 years ago
|
||
Comment on attachment 8853619 [details] [diff] [review]
tracking_flags.patch
Review of attachment 8853619 [details] [diff] [review]:
-----------------------------------------------------------------
::: extensions/TrackingFlags/lib/Flag/Visibility.pm
@@ +76,3 @@
> }
>
> + delete $params->{$_} for qw( is_active component product );
instead of ignoring is_active here, it would be better to remove it from the calling site.
Attachment #8853619 -
Flags: review?(glob) → review-
Reporter | ||
Comment 24•8 years ago
|
||
Comment on attachment 8853628 [details] [diff] [review]
field-descs.patch
Split into https://bugzilla.mozilla.org/show_bug.cgi?id=1355127
Attachment #8853628 -
Attachment is obsolete: true
Reporter | ||
Comment 25•8 years ago
|
||
Attachment #8853622 -
Attachment is obsolete: true
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8853621 [details] [diff] [review]
field_obj_cache.patch
Split into bug 1355137
Attachment #8853621 -
Attachment is obsolete: true
Reporter | ||
Comment 27•8 years ago
|
||
Comment on attachment 8853624 [details] [diff] [review]
trick_taint.patch
Split off into Bug 1355142 with a fresh patch to review.
Attachment #8853624 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Alias: bmo-slow
Reporter | ||
Updated•6 years ago
|
Assignee: dylan → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•