1351895 - (bmo-slow) A large collection of slow things

Status: NEW

(bugzilla.mozilla.org :: General, task, P2)

Description

[Dylan Hardison [:dylan] (he/him)]

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

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Nothing here is particularly actionable here, so in second thought this bug can be open.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Attachment: new_from_where.patch

I think we've wanted this for a while -- it's a bit limited but it does reduce the number of SELECTS.

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: tracking_flags.patch

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Attachment: active_custom_fields.patch

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?

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Attachment: field_obj_cache.patch

This one is a pretty obvious in hindsight.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Attachment: fetchall.patch

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.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Attachment: trick_taint.patch

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);

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: class_xs_accessor.patch

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.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: html_quote.patch

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.

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Attachment: field-help.patch

I rewrote a template in perl... using [% RAWPERL %].
The template was very expensive, it's now not expensive at all.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Attachment: field-descs.patch

This is not the sort of optimization that is... very nice looking, but the performance is difficult to ignore.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

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

[:glob ✱]

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

Comment 14

[:glob ✱]

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

Comment 15

[:glob ✱]

Comment on attachment 8853621 [details] [diff] [review]
field_obj_cache.patch

Review of attachment 8853621 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 16

[:glob ✱]

Comment on attachment 8853622 [details] [diff] [review]
fetchall.patch

Review of attachment 8853622 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 17

[:glob ✱]

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

Comment 18

[:glob ✱]

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 ]

Comment 19

[:glob ✱]

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.

Comment 20

[:glob ✱]

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.

Comment 21

[:glob ✱]

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;

Comment 22

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is being worked on so marking as P1.

Comment 23

[:glob ✱]

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.

Comment 24

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8853628 [details] [diff] [review]
field-descs.patch

Split into https://bugzilla.mozilla.org/show_bug.cgi?id=1355127

Comment 25

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8853622 [details] [diff] [review]
fetchall.patch

Split into bug 1355134

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8853621 [details] [diff] [review]
field_obj_cache.patch

Split into bug 1355137

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8853624 [details] [diff] [review]
trick_taint.patch

Split off into Bug 1355142 with a fresh patch to review.