498318 - Speed up field-descs.none.tmpl

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Bradley Baetz (:bbaetz)]

Attachment: Patch

Loading field-descs.none.tmpl is slow. format_comment is used in a sub-template, per comment, so this is loaded per comment.

This template doesn't need any of the bug fields, only terms, so just load variables.html.tmpl.

Improvements - 7-10% (after loading libraries) on a bug with 2 comments, 80% (98.7 secs/100 -> 19.8) for a bug with 57 comments

There are better potential fixes (like processing all the comments in the template at the end), but this is quick and simple.

Comment 1

[Bradley Baetz (:bbaetz)]

> Improvements - 7-10% (after loading libraries) on a bug with 2 comments, 80%
> (98.7 secs/100 -> 19.8) for a bug with 57 comdents

actually, only -> 47.3 seconds, so 'only' a 52% improvement

Comment 2

[Bradley Baetz (:bbaetz)]

Attachment: v2

This cuts it down to 45.1 seconds. Not huge, especially since all my comments are from the same user, but it can't hurt.

Comment 3

[Max Kanat-Alexander]

Probably a better solution would be to make field-descs fast, no? Cache the "fields" data in something other than just the global template namespace--like in request_cache or something. Or cache any parameterless calls to get_fields() in Bugzilla.pm.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 383252 [details] [diff] [review]
v2

format_comment uses get_status, which is defined in field-descs.

I think we'll have to cache the "bug_fields" object and just re-use that. Probably in Bugzilla.request_cache.

Comment 5

[Bradley Baetz (:bbaetz)]

(In reply to comment #3)
> Probably a better solution would be to make field-descs fast, no? 

Yeah, but I wanted something to backport. The calls to get_status are also slow - I'm going to look into somehow getting the hash back from TT and storing those locally.

> Cache the
> "fields" data in something other than just the global template namespace--like
> in request_cache or something. Or cache any parameterless calls to get_fields()
> in Bugzilla.pm.

We really only need to call it for custom fields, but I think that moving the INCLUDE from messages.html.tmpl to the single message that needs it will be the simplest for this

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Yeah, but I wanted something to backport. The calls to get_status are also slow
> - I'm going to look into somehow getting the hash back from TT and storing
> those locally.

  How are they slow? Are they hitting the DB somehow?

> We really only need to call it for custom fields, but I think that moving the
> INCLUDE from messages.html.tmpl to the single message that needs it will be the
> simplest for this

  Or you could make it a PROCESS....

Comment 7

[Bradley Baetz (:bbaetz)]

(In reply to comment #6)
>   How are they slow? Are they hitting the DB somehow?

foreach comment
  quoteURLs filter
    foreach match
      use template_inner for get_status
        load messages.html.tmpl
          load field-descs
            get fields from the DB
      repeat for get_resolution
        note that we clear the inner template's context, so this starts again from scratch

Even if I move the INCLUDE, loading the extra template is extra overhead. Not as much as calling it for each comment even if it doesn't match, but its very noticeable. This patch (and the few others) gives an 80% speedup...

>   Or you could make it a PROCESS....

That won't work - we want the variables from the subtemplate to stay around. Plus it wouldn't help - the issue is calling into templates entirely.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> >   Or you could make it a PROCESS....
> 
> That won't work - we want the variables from the subtemplate to stay around.
> Plus it wouldn't help - the issue is calling into templates entirely.

  I think you have PROCESS and INCLUDE backwards.

Comment 9

[Bradley Baetz (:bbaetz)]

(In reply to comment #8)

>   I think you have PROCESS and INCLUDE backwards.

Err, right. But its a PROCESS in messages.html.tmpl anyway (and has to be to work), so thats not the issue.

Comment 10

[Bradley Baetz (:bbaetz)]

So I can't really lazy load the field_descs stuff because the whole point of that is that the template can override the descriptions.

I'll have to think of something else...

Comment 11

[Max Kanat-Alexander]

[% USE Bugzilla %]
[% IF NOT Bugzilla.request_cache.template_global_field_descs_bug_fields.defined %]
   # blah blah blah
[% END %]
[% SET bug_fields =  Bugzilla.request_cache.template_global_field_descs_bug_fields %]

Comment 12

[Max Kanat-Alexander]

In fact, instead of that, how about we just create a bug_fields() variable or something for templates, during Template->create, and cache the result in $template? We only ever create two template objects during a request.

Comment 13

[Bradley Baetz (:bbaetz)]

Your first option doesn't help, since it still does the load for each sub template.

Comment 14

[Vitaly Fedrushkov]

(In reply to comment #7)
Sadly, looks like gettext is being reinvented.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #13)
> Your first option doesn't help, since it still does the load for each sub
> template.

  You mean the option from comment 7? There wouldn't be any reloading there.

Comment 16

[Bradley Baetz (:bbaetz)]

(In reply to comment #15)
> (In reply to comment #13)
> > Your first option doesn't help, since it still does the load for each sub
> > template.
> 
>   You mean the option from comment 7? There wouldn't be any reloading there.

No, I meant comment 11 (as opposed to comment 12, which is sort of what I've done, but its not that simple because we have to allow the templates to override for field_descs)

Comment 17

[Max Kanat-Alexander]

Attachment: v3

Here, I got tired of explaining myself, and this was short. :-)

Comment 18

[Bradley Baetz (:bbaetz)]

Comment on attachment 383848 [details] [diff] [review]
v3

:) Yeah, this is simpler, and even backportable.

Let me benchmark it over my other approach tonight and see how it all ends up.

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 383848 [details] [diff] [review]
v3

OK, this moves my 57 comment bug from 1.63 seconds to 1.08, plus its simpler then what I had. We still loop through the fields each time that template is loaded, but thats a small issue.

Comment 20

[Bradley Baetz (:bbaetz)]

The 3 comment one went from 300ms to 275ms (+/- 5ms)

On 3.4, the large comment test goes from 1.47s to 985ms
3.2: 765ms to 630ms
3.0: 691ms to 504ms

(times +/- 10ms)

The other fixes can go into a followup bug. This is simple, and a big enough win that I think it should go back to at least 3.2, and probably 3.0 as well. Thoughts?

Comment 21

[Bradley Baetz (:bbaetz)]

Attachment: backport to 3.2

3.4 and head can use the same patch

Comment 22

[Bradley Baetz (:bbaetz)]

Attachment: backport to 3.0

Comment 23

[Frédéric Buclin]

No, 3.0 is restricted to security fixes only. Perf fixes are excluded. Also, backports need to be reviewed before getting approvals.

Comment 24

[Bradley Baetz (:bbaetz)]

Max, this one is your patch - do you want to check it in or do you want me to?

Comment 25

[Frédéric Buclin]

No answer for 3 days; do it yourself.

Comment 26

[Max Kanat-Alexander]

tip:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.101; previous revision: 1.100
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.33; previous revision: 1.32
done


3.4:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.99.2.1; previous revision: 1.99
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.31.2.2; previous revision: 1.31.2.1
done

3.2:

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.89.2.3; previous revision: 1.89.2.2
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.25.2.3; previous revision: 1.25.2.2
done