731562 - Cache the global/user.html.tmpl template

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frédéric Buclin]

Attachment: patch, v1

We waste a lot of time calling global/user.html.tmpl for each attachment author and each commenter despite you often have the same few people involved in a bug. This template should be cached in the User object and reused when this attacher/commenter is present again. This is now possible because I shared the same User object in bug 731559.

I also added a helper method Bugzilla::User::groups_with_icon() which returns groups having an icon. As groups are cached in User objects and that we now share User objects, this is another perf win.

On my testing with the bmo DB, this saves me 2 seconds, from 14.8 seconds down to 12.6 seconds when viewing a large bug report such as 38862!

Comment 1

[David Lawrence [:dkl]]

Comment on attachment 601584 [details] [diff] [review]
patch, v1

r- per our discussion in IRC about creating a more generic scratch space for these type of values so that other code can take advantage. IMO is also better than extending a well defined object.

dkl

Comment 2

[Frédéric Buclin]

Attachment: patch, v2

Using Bugzilla->request_cache is indeed cleaner and lets us share it between the attachments table and comments. :)

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 601705 [details] [diff] [review]
patch, v2

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

::: template/en/default/attachment/list.html.tmpl
@@ +44,5 @@
> +[% USE Bugzilla %]
> +[% bz_cache = Bugzilla.request_cache %]
> +[% UNLESS bz_cache.exists("vcards") %]
> +  [% bz_cache.vcards = {} %]
> +[% END %]

Could we put this in a bug/show-header.html.tmpl or some other suitably higher level template instead of having to repeat this section in each template we want to use this for? Or we could pre-initialize it in Bugzilla.pm if that would be better.

Comment 4

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #3)
> Could we put this in a bug/show-header.html.tmpl or some other suitably
> higher level template instead of having to repeat this section in each
> template we want to use this for? 

I cannot put it in bug/show-header.html.tmpl nor in bug/edit.html.tmpl, because neither bug/process/midair.html.tmpl nor bug/show-multiple.html.tmpl call them. If we can prove later that there are other places where this would be useful, we can move this elsewhere at that moment.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 601705 [details] [diff] [review]
patch, v2

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

::: template/en/default/attachment/list.html.tmpl
@@ +43,5 @@
>  
> +[% USE Bugzilla %]
> +[% bz_cache = Bugzilla.request_cache %]
> +[% UNLESS bz_cache.exists("vcards") %]
> +  [% bz_cache.vcards = {} %]

Won't this just auto-vivify when you ask for it?

@@ +100,5 @@
> +            [% UNLESS bz_cache.vcards.$attacher_id %]
> +              [% bz_cache.vcards.$attacher_id = BLOCK %]
> +                [% INCLUDE global/user.html.tmpl who = attachment.attacher %]
> +              [% END %]
> +            [% END %]

Why not just put this into global/user.html.tmpl? Is it purely the act of running an INCLUDE that's slow?

Comment 6

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #5)
> Won't this just auto-vivify when you ask for it?

I thought about that too, but it remains undefined.


> Why not just put this into global/user.html.tmpl? Is it purely the act of
> running an INCLUDE that's slow?

INCLUDE is slower than PROCESS, yes, and I want to keep control on the cache on the caller side.

Comment 7

[Frédéric Buclin]

Attachment: patch, v3

OK, I did some more testing, trying different solutions, and this one is the most optimized one. A few results of my study (may be helpful for future development):

- Using PROCESS global/user.html.tmpl instead of INCLUDE doesn't help. In my testing, the difference was constantly negligible and in the noise.

- Putting the cache in global/user.html.tmpl itself makes us loose half of the perf win. This means we have everything in a central place, but there is a time penalty as we still have to call this template many times. That's not the goal.

- The winner is clearly this patch, i.e. letting the caller cache the data if this makes sense. Note that I optimized the code a bit as I moved the cache definition in Template.pm. At least this part is centralized.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 601909 [details] [diff] [review]
patch, v3

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

Overall this is a great idea. Just a couple of suggested changes but otherwise looks good and improves load time on my test instance.

::: Bugzilla/Template.pm
@@ +918,5 @@
> +            'user_cache' => sub {
> +                my $cache = Bugzilla->request_cache->{user_cache} ||= {};
> +                return $cache;
> +            },
> +

I would still like to see this more general purpose as I can see using this in other places in the future. So maybe we can change it to:

                # A general purpose cache to store rendered templates for reuse.
		# Make sure to not mix language-specific data.
		'template_cache' => sub {
		    my $cache = Bugzilla->request_cache->{template_cache} ||= {};
                    $cache->{users} ||= {};
		    return $cache;
		},

::: template/en/default/attachment/list.html.tmpl
@@ +90,5 @@
>                [%- attachment.attached FILTER time %]</a>,
>  
> +            [%# No need to recreate the exact same template if we already have it. %]
> +            [% attacher_id = attachment.attacher.id %]
> +            [% UNLESS user_cache.$attacher_id %]

With my suggested change it would be 

           [% UNLESS template_cache.users.$attacher_id %]
...

::: template/en/default/bug/comments.html.tmpl
@@ +171,5 @@
>  
>          <span class="bz_comment_user">
> +          [%# No need to recreate the exact same template if we already have it. %]
> +          [% commenter_id = comment.author.id %]
> +          [% UNLESS user_cache.$commenter_id %]

Same here.
         [% UNLESS template_cache.users.$commenter_id %]
...

Comment 9

[Frédéric Buclin]

Attachment: patch, v4

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 602084 [details] [diff] [review]
patch, v4

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

Looks good and works as expected. r=dkl

Comment 11

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Template.pm
modified Bugzilla/User.pm
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/bug/comments.html.tmpl
Committed revision 8142.