Speed up field-descs.none.tmpl

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bbaetz, Assigned: Max Kanat-Alexander)

Tracking

({perf})

Bugzilla 3.2
Bug Flags:
approval +
approval3.4 +
blocking3.4 -
approval3.2 +
approval3.0 -

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 383245 [details] [diff] [review]
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.
Flags: blocking3.4?
Attachment #383245 - Flags: review?(mkanat)
(Reporter)

Comment 1

8 years ago
> 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
(Reporter)

Comment 2

8 years ago
Created attachment 383252 [details] [diff] [review]
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.
Assignee: create-and-change → bbaetz
Attachment #383245 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #383252 - Flags: review?(mkanat)
Attachment #383245 - Flags: review?(mkanat)
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Updated

8 years ago
Severity: critical → major
Flags: blocking3.4? → blocking3.4-
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 3.4
Version: 3.3.4 → 3.0
(Assignee)

Updated

8 years ago
Attachment #383252 - Flags: review?(mkanat) → review-
(Assignee)

Comment 4

8 years ago
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.
(Reporter)

Comment 5

8 years ago
(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
(Assignee)

Comment 6

8 years ago
(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....
(Reporter)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
(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.
(Reporter)

Comment 9

8 years ago
(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.
(Reporter)

Comment 10

8 years ago
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...
(Assignee)

Comment 11

8 years ago
[% 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 %]
(Assignee)

Comment 12

8 years ago
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.
(Reporter)

Comment 13

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

Comment 14

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

Comment 15

8 years ago
(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.
(Reporter)

Comment 16

8 years ago
(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)
(Assignee)

Comment 17

8 years ago
Created attachment 383848 [details] [diff] [review]
v3

Here, I got tired of explaining myself, and this was short. :-)
Attachment #383252 - Attachment is obsolete: true
Attachment #383848 - Flags: review?(bbaetz)
(Reporter)

Comment 18

8 years ago
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.
(Reporter)

Updated

8 years ago
Attachment #383848 - Flags: review?(bbaetz) → review+
(Reporter)

Comment 19

8 years ago
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.
(Reporter)

Comment 20

8 years ago
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?
Flags: approval?
Flags: approval3.4?
Flags: approval3.2?
Flags: approval3.0?
(Reporter)

Updated

8 years ago
Summary: Speed up format_comment → Speed up field-descs.none.tmpl
(Reporter)

Comment 21

8 years ago
Created attachment 383898 [details] [diff] [review]
backport to 3.2

3.4 and head can use the same patch
(Reporter)

Comment 22

8 years ago
Created attachment 383899 [details] [diff] [review]
backport to 3.0

Comment 23

8 years ago
No, 3.0 is restricted to security fixes only. Perf fixes are excluded. Also, backports need to be reviewed before getting approvals.
Flags: approval3.0? → approval3.0-
(Assignee)

Updated

8 years ago
Flags: approval?
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
Target Milestone: Bugzilla 3.4 → Bugzilla 3.2
(Assignee)

Updated

8 years ago
Attachment #383898 - Flags: review+
(Reporter)

Comment 24

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

Comment 25

8 years ago
No answer for 3 days; do it yourself.
(Assignee)

Comment 26

8 years ago
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
Assignee: bbaetz → mkanat
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.