Last Comment Bug 731850 - Use cached templates to improve performance
: Use cached templates to improve performance
Status: RESOLVED FIXED
: perf
Product: bugzilla.mozilla.org
Classification: Other
Component: Extensions: InlineHistory (show other bugs)
: Production
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Byron Jones ‹:glob›
:
Mentors:
Depends on: 731562 732189
Blocks: 685146 734010
  Show dependency treegraph
 
Reported: 2012-02-29 16:03 PST by Frédéric Buclin
Modified: 2012-03-22 10:09 PDT (History)
3 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (7.89 KB, patch)
2012-02-29 19:25 PST, Frédéric Buclin
no flags Details | Diff | Review
patch, v1.1 (8.96 KB, patch)
2012-03-01 05:06 PST, Frédéric Buclin
no flags Details | Diff | Review

Description Frédéric Buclin 2012-02-29 16:03:39 PST
Upstream in bug 731562, I cache global/user.html.tmpl on a per user basis as the exact same template is reused several times in the attachments table and in comments. This improves load time of large bugs *a lot*. To improve performance further on bmo, the Inline History extension should use it too.

Here are some numbers, using the bmo DB I have in hands, while viewing 38862:

Current:                 14.8 seconds

+ patch from bug 731562: 12.6 seconds

+ this bug on top of it: 11.1 seconds

3.7 seconds less!! I'm done with my patch, but I'm still looking if there are some easy perf wins here and there (already found a few in Extension.pm).
Comment 1 Frédéric Buclin 2012-02-29 18:22:34 PST
I have improved my patch a bit and now results are as follow:

Current:                 14.8 seconds

+ patch from bug 731562: 12.6 seconds

+ this bug on top of it: 7.7 seconds (!!)

/me dances in the street \o/
Comment 2 Frédéric Buclin 2012-02-29 19:25:50 PST
Created attachment 601852 [details] [diff] [review]
patch, v1

Here is a summary of improvements I made:

- A lot of time is wasted in get_bug_link() to get the bug status and resolution, independently of whether this extension is turned on or off. The reason is that get_text() reloads the template all the time to get the same information again and again. This problem has been fixed in 4.2 in bug 595410, but meanwhile, I cache the data. As bmo uses English only, this is totally safe. The win is of the order of 2 seconds for this single change, on my machine.

- In Extension.pm, you were loading Bugzilla->user and Bugzilla->dbh before making sure it was being called by the right template. This meant more than 2000 calls for nothing. Now only one call is made! $user and $user->settings are *always* defined, even for logged out users or for users who never edited their user prefs (it's so since 2.20, IIRC), so these checks are useless. Also, the check for bug/show-header.html.tmpl to set ih_activity = 1 is useless. See how I fixed hook/bug/show-header-end.html.tmpl at the end of the patch.

- Most of the user objects are already available from Bug->comments. All attachment authors are also commenters (due to the "created attachment" comment), and so only those who made a change to the bug without commenting still need a user object.

- Same about attachments. Attachment objects are already available in Bug->attachments.

- Due to security checks, each bug is going to be checked one by one by $user->can_see_bug() when linkifying them in comments, duplicates list, dependency lists, and in the inline history. Calling visible_bugs() here caches them all at once, making subsequent checks very fast.

- hook/bug/comments-aftercomments.html.tmpl uses the same cache as the one defined in bug 731562. This is another major improvement to the extension. This means that you need to backport bug 731562 first in order for it to work.

- Your calls to display_value() are useless. There no definition in the display_value() macro in field-descs.none.tmpl for user fields, nor with any other field either. So this loads this template several tens of times for nothing, and we all know how slow this template is, despite template_var().
Comment 3 Frédéric Buclin 2012-03-01 05:06:47 PST
Created attachment 601931 [details] [diff] [review]
patch, v1.1

This patch has been updated to work with the new cache introduced in bug 731562, and also to refactor comments-aftercomments.html.tmpl a bit to avoid useless calls to get the same information. One notable example is |FILTER time| which is slow and should be called as few as possible. Another 0.1 second win.

At this point, I see no other obvious bottleneckin this extension, besides the fact that 1/3 of the time is wasted in the remaining calls to |FILTER time|. The time field is mosty used to find the correct comment in the bug report, i.e. is used as an identifier. More efficient would be to use the internal comment ID, which is unique across all bugs. But that's another story and is out of the scope of this bug.
Comment 4 Byron Jones ‹:glob› 2012-03-01 09:12:01 PST
(In reply to Frédéric Buclin from comment #3)
> More efficient would be to use the
> internal comment ID, which is unique across all bugs. But that's another
> story and is out of the scope of this bug.

you can't use the comment id because changes aren't always made at the same time as a comment.
Comment 5 Byron Jones ‹:glob› 2012-03-01 09:28:06 PST
Comment on attachment 601931 [details] [diff] [review]
patch, v1.1

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

thanks for the work here!  most of this looks good.

this review is just from a quick read of the code; i'll work through a popper test and review later.

::: Bugzilla/Template.pm
@@ +333,5 @@
> +    my $status_cache = Bugzilla->request_cache->{bug_status} ||= {};
> +
> +    # bmo only uses English, so that's fine.
> +    $title = $status_cache->{$bug->bug_status} ||=
> +      get_text('get_status', { status => $bug->bug_status });

this extension isn't bmo-specific, there are other sites which run it.
that said, i'm happy for it to be english only, which means the display_value change should also be ok.

::: extensions/InlineHistory/Extension.pm
@@ +31,4 @@
>  use Bugzilla::Constants;
>  use Bugzilla::Attachment;
>  
> +our $VERSION = '1.4.1';

1.5, i don't want minor version numbers here.

@@ +69,5 @@
> +    my $user_cache = {};
> +    $user_cache->{$_->{author}->login} = $_->{author} foreach @{$bug->comments};
> +
> +    my %attachment_cache;
> +    $attachment_cache{$_->id} = $_ foreach @{$bug->attachments};

i'm not a fan of using $_, please use a fully expanded foreach loop in both locations.

@@ +116,5 @@
> +                    my @buglist = split(/[\s,]+/, $change->{$what});
> +                    # Sanity check, as they are going to be passed
> +                    # as is to the DB.
> +                    @buglist = grep { $_ && $_ =~ /^\d+$/} @buglist;
> +                    $bug_cache{$_} = 1 foreach @buglist;

expand foreach here too.
Comment 6 Frédéric Buclin 2012-03-01 09:35:00 PST
(In reply to Byron Jones ‹:glob› from comment #5)
> i'm not a fan of using $_, please use a fully expanded foreach loop in both
> locations.

We use this at many places within the Bugzilla code. It's much cleaner that the whole foreach loop. If you want it differently, please do it. I'm done with this patch (a.k.a. I see no reason why $_ isn't fine, so you won't make me change my mind on this). :)
Comment 7 David Lawrence [:dkl] 2012-03-01 14:33:48 PST
Comment on attachment 601931 [details] [diff] [review]
patch, v1.1

As glob is the author I will concede the review to him. FWIW this does give some performance boost in my testing so the changes are working some.
Comment 8 Byron Jones ‹:glob› 2012-03-01 21:06:05 PST
(In reply to Frédéric Buclin from comment #6)
> We use this at many places within the Bugzilla code. It's much cleaner that
> the whole foreach loop. If you want it differently, please do it. I'm done
> with this patch (a.k.a. I see no reason why $_ isn't fine, so you won't make
> me change my mind on this). :)

taking.  i may fold these changes into bug 731986 depending on the results over there.
Comment 9 Byron Jones ‹:glob› 2012-03-05 23:12:57 PST
> ::: Bugzilla/Template.pm
> @@ +333,5 @@
> > +    my $status_cache = Bugzilla->request_cache->{bug_status} ||= {};
> > +
> > +    # bmo only uses English, so that's fine.
> > +    $title = $status_cache->{$bug->bug_status} ||=
> > +      get_text('get_status', { status => $bug->bug_status });

isn't this change safe to go upstream?

we're only caching in the request_cache, and the current language isn't going to change mid-request.
Comment 10 Frédéric Buclin 2012-03-06 03:36:24 PST
(In reply to Byron Jones ‹:glob› from comment #9)
> isn't this change safe to go upstream?

No need. This problem has already been fixed in 4.2 and newer.
Comment 11 Byron Jones ‹:glob› 2012-03-07 09:46:43 PST
thanks lpsolit!

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.0/
modified Bugzilla/Template.pm
modified extensions/InlineHistory/Config.pm
modified extensions/InlineHistory/Extension.pm
modified extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/bug/comments-comment_banner.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/bug/show-header-end.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/global/setting-descs-settings.none.tmpl
modified extensions/InlineHistory/web/inline-history.js
modified extensions/InlineHistory/web/style.css
modified extensions/Splinter/Extension.pm
Committed revision 8093.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.2/
modified extensions/InlineHistory/Config.pm
modified extensions/InlineHistory/Extension.pm
modified extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/bug/comments-comment_banner.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/bug/show-header-end.html.tmpl
modified extensions/InlineHistory/template/en/default/hook/global/setting-descs-settings.none.tmpl
modified extensions/InlineHistory/web/inline-history.js
modified extensions/InlineHistory/web/style.css
modified extensions/Splinter/Extension.pm
Committed revision 8080.

Note You need to log in before you can comment on or make changes to this bug.