Open
Bug 1351895
(bmo-slow)
Opened 7 years ago
Updated 5 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•7 years ago
|
||
Nothing here is particularly actionable here, so in second thought this bug can be open.
Group: bmo-infra
Reporter | ||
Comment 2•7 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•7 years ago
|
||
Attachment #8853619 -
Flags: review?(glob)
Reporter | ||
Comment 4•7 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•7 years ago
|
||
This one is a pretty obvious in hindsight.
Attachment #8853621 -
Flags: review?(glob)
Reporter | ||
Comment 6•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Priority: P1 → P2
Comment 23•7 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•7 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•7 years ago
|
||
Comment on attachment 8853622 [details] [diff] [review] fetchall.patch Split into bug 1355134
Attachment #8853622 -
Attachment is obsolete: true
Reporter | ||
Comment 26•7 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•7 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•6 years ago
|
Alias: bmo-slow
Updated•5 years ago
|
Type: enhancement → task
Reporter | ||
Updated•5 years ago
|
Assignee: dylan → nobody
You need to log in
before you can comment on or make changes to this bug.
Description
•