797636 - Improve performance for buglists

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Frédéric Buclin]

Attachment: patch, v1

In the spirit of bug 795650 and bug 796072, I analyzed large buglists using Devel::NYTProf to understand where we waste time. The attached patch fixes several key places to execute as fast as possible:

- First of all, Bugzilla->foo methods are called several hundreds of thousands times in a single buglist, especially Bugzilla->params and Bugzilla->request_cache, but also Bugzilla->user and Bugzilla->local_timezone, etc... One of the reasons is that |FILTER html| calls Bugzilla::Util::html_quote() which calls Bugzilla->params->{utf8}. To make the code faster, I replaced

  my $class = shift;
  $class->{foo} ||= <some code>;
  return $class->{foo};

by

  return $_[0]->{foo} ||= <some code>;

What is slow in Perl is to copy a variable into another one. By using $_[0] directly instead of copying it into $class and to return $_[0]->{foo} immediately at the same time it's set makes a significant difference. That's what I did in Bugzilla.pm.

- |use Bugzilla| is loading Bugzilla::Config which was loading Bugzilla::Install::Filesystem to load fix_file_permissions(), but this method is only used in write_params(), i.e. not very often (only when you edit parameters). As loading Bugzilla::Install::Filesystem is slow, I moved it into write_params() directly, as we don't need it most of the time.

- Bugzilla::Util::html_quote() is called every time a template has |FILTER html|, which is the case for 99.999% of data displayed in templates. To make it faster, I copied the content of Template::Filters::html_filter() into html_quote() directly, to avoid copying strings back and forth all the time between both methods.

- display_value() is called for almost each cell of the buglist to get localized values (even if there is only one language installed). This method was rather slow because it calls Bugzilla::Util::template_var() which was calling Bugzilla->template_inner way too often to get the current language. This information is already available in Bugzilla->request_cache->{template_current_lang}, and so it was useless to call Bugzilla->template_inner all the time, which was the bottleneck.

- buglist.cgi truncates most values and so I did a minor optimization in $Template::Stash::SCALAR_OPS->{ truncate } in Bugzilla/Template.pm to not bother defining $ellipsis unless required.

- Template::Context::filter() has an optimization which has been disabled upstream by default. To re-enable it, you must pass a filter alias when calling this method so that the filter is cached instead of letting Template::Filters look for and reload the filter all the time. That's why I override this method in Bugzilla::Template::Context::filter(). This makes a significant difference as there is no need to call Template::Filters::fetch() for each |FILTER foo| anymore.

- In list/table.html, I did various improvements to make the template load faster. If we set maxlength => 0 instead of letting it undefined when not needed, we avoid many calls to Template::Stash::undefined(). Also, .search() is slightly faster than .match() because .search() simple returns true/false while .match() returns an array of matching strings, which we ignore. I also store abbrev.$column into col_abbrev to avoid several hundreds of thousands calls to Template::Stash::XS::get() which is what consumes the most time in large buglists. And I moved the call to the time_summary_line block outside the loop as we want to display bug summary stuff only when all bugs have been processeed. This brings no perf improvement as loop.last() seems pretty fast, but it was a logical move.

- Calls to bug/time.html to format timetracking fields were pretty slow due to all the formatting calls made in this template. I optimized this code a bit (you need timetracking fields to be displayed to see a difference).


Here are some interesting stats with Devel::NYTProf enabled:

                              | Without the patch |  With the patch  | Diff
---------------------------------------------------------------------------------
size of nytprof.out           |       132 MB      |      91 MB       |  -31%
time reported by NYTProf      | 30.5s (of 45.8s)  | 25.1s (of 35.2s) |  -18%/-23%
# statements                  |   11'233'003      |  7'479'216       |  -33%
# subroutine calls            |    4'158'779      |  3'545'039       |  -15%
Template::Stash::XS::get      |        12.3s      |      9.66s       |  -21%
Template::Context::filter     |        2.92s      |      1.80s       |  -38%
Bugzilla::Util::html_quote    |        3.09s      |      2.05s       |  -34%
Bugzilla::Util::display_value |        2.98s      |      1.04s       |  -65%
Bugzilla->params              |        950ms      |      514ms       |  -46%
Bugzilla->request_cache       |        923ms      |      322ms       |  -65%

With Devel::NYTProf disabled, to get more accurate timing:

page loading time             |        11.7s      |      10.5s       |  -10%

The 10% win to load the page is less impressive than the numbers reported by Devel::NYTProf, but is not negligible either. Also, all pages will benefit from the optimizations made here.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1.1

Fix bitrot due to bug 797883.

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 668399 [details] [diff] [review]
patch, v1.1

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

Small nits of whitespace but overall looks good and works well in my testing. Look forward to getting this on BMO as well. r=dkl

My less than scientific timings using 'siege'. Each row is running a large buglist.cgi call 5 times with 1 client and recording the average time of all 5.

mod_cgi:

without patch:
2.88
2.85
2.94
----
2.89 avg

with patch:
2.77
2.73
2.79
----
2.76 avg

mod_perl:

without patch:
2.35 (unprimed)
2.18
2.08
----
2.20 avg

with patch:
2.08 (unprimed)
1.98
1.78
----
1.94 avg

::: Bugzilla/Template.pm
@@ -545,5 @@
>  $Template::Stash::SCALAR_OPS->{ truncate } = 
>    sub {
>        my ($string, $length, $ellipsis) = @_;
> -      $ellipsis ||= "";
> -      

nit: whitespace

@@ -550,2 @@
>        return $string if !$length || length($string) <= $length;
> -      

nit: whitespace

::: template/en/default/bug/time.html.tmpl
@@ -5,4 @@
>    # This Source Code Form is "Incompatible With Secondary Licenses", as
>    # defined by the Mozilla Public License, v. 2.0.
>    #%]
> -                                      

nit: whitespace

Comment 3

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #2)
> Comment on attachment 668399 [details] [diff] [review]
> patch, v1.1
> 
> Review of attachment 668399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Small nits of whitespace but overall looks good and works well in my
> testing. Look forward to getting this on BMO as well. r=dkl

Ugh, ignore the comments on whitespace as they are being removed not added. Need more coffee (or less for that matter) :)

dkl

Comment 4

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla.pm
modified Bugzilla/Config.pm
modified Bugzilla/Template.pm
modified Bugzilla/Util.pm
modified Bugzilla/Template/Context.pm
modified template/en/default/filterexceptions.pl
modified template/en/default/bug/time.html.tmpl
modified template/en/default/list/table.html.tmpl
Committed revision 8475.