Closed Bug 498309 Opened 10 years ago Closed 10 years ago

Calling get_text in quoteUrls over and over is very slow (show_bug.cgi page with many comments is slow)

Categories

(Bugzilla :: Creating/Changing Bugs, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: bbaetz, Assigned: mkanat)

References

Details

(Keywords: perf)

Attachments

(3 files, 11 obsolete files)

11.98 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
11.54 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
3.59 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — Splinter Review
quoteUrls is a filter, which means that its run for each comment. One of the things that is done per comment is to call |get_text('term', { term => 'bug' })| to match 'bug 12345' type comments.

This is slow for a number of reasons - not only do we have to call a template, but get_text uses messages.html.tmpl, which loads field-descs.none.tmpl, which calls into the DB to get the fields, and then loops over each field. For each comment.

There are a number of issues with this, but the simplest fix is to cache the term (per language, even though in this case its not needed)

timing for 10 runs of |do 'show_bug.cgi'; $Bugzilla::_request_cache = {};|, on a bug with 57 comments, when not logged in, redirecting output to /dev/null, with one extra run before hand to load all the modules:

trunk: 15 s
with cache: 9.93s
with different template: 10.24s
with cache + different template: 9.77s

(times +/- 0.1s)

FWIW:

removing setting field_descs/bug_fields from field-descs.none.tmpl: 6.0 s
removing ... with this patch: 5.7s

but that's a separate bug....

(bug 915 has 311 comments, and takes 14 seconds to load for me, about 11 of which is before any response)

I thought about doing this with get_text directly, but some of that may need the fields (importxml uses it). Most get_text users are in checksetup where the actual DB changes will take all the time, so its not a big issue.

this bug dates back to 3.0, although I've only tested with trunk.
Flags: blocking3.4?
Flags: blocking3.2.4?
Flags: blocking3.0.9?
Attachment #383235 - Flags: review?(mkanat)
Assignee: create-and-change → bbaetz
Status: NEW → ASSIGNED
Not a blocker, but important perf issue for sure.
Severity: critical → major
Flags: blocking3.4?
Flags: blocking3.4-
Flags: blocking3.2.4?
Flags: blocking3.2.4-
Flags: blocking3.0.9?
Flags: blocking3.0.9-
OS: Linux → All
Hardware: x86 → All
Attachment #383235 - Flags: review?(mkanat) → review-
Comment on attachment 383235 [details] [diff] [review]
Patch

>Index: Bugzilla/Util.pm
>+    my ($lang) = include_languages();

  This won't work if you ever use a term in an email, FWIW.

  Also, include_languages is not cached, FWIW.

>+    my $cache = Bugzilla->request_cache;
>+
>+    if (!defined $cache->{terms}->{$lang}->{$term}) {
>+        my $message;
>+        my $template = Bugzilla->template_inner;

  You didn't initialize template_inner with the language? (template_inner works in a bad way that should be fixed, which requires that we set and then unset the language when we use it.) That might be OK, though, in this context....

>Index: template/en/default/global/term.txt.tmpl
>+[% PROCESS "global/variables.none.tmpl" %]
>+[% terms.$term FILTER html %]

  This is a .txt template, so you should be filtering txt. Or it should be a .html template.
(In reply to comment #2)
> (From update of attachment 383235 [details] [diff] [review])
> >Index: Bugzilla/Util.pm
> >+    my ($lang) = include_languages();
> 
>   This won't work if you ever use a term in an email, FWIW.

True, but it won't work now, because those are already using the inner template - we'd need Bugzilla->inner_inner_template or something.

> 
>   Also, include_languages is not cached, FWIW.

My assumption is that it can change - if it can't then I don't need $lang in the cache.

> >+    my $cache = Bugzilla->request_cache;
> >+
> >+    if (!defined $cache->{terms}->{$lang}->{$term}) {
> >+        my $message;
> >+        my $template = Bugzilla->template_inner;
> 
>   You didn't initialize template_inner with the language? (template_inner works
> in a bad way that should be fixed, which requires that we set and then unset
> the language when we use it.) That might be OK, though, in this context....

Yeah, thats a bug.

> >Index: template/en/default/global/term.txt.tmpl
> >+[% PROCESS "global/variables.none.tmpl" %]
> >+[% terms.$term FILTER html %]
> 
>   This is a .txt template, so you should be filtering txt. Or it should be a
> .html template.

should actually be FILTER NONE.

That said, let me try to come up with a better way of doing this all.
(In reply to comment #3)
> My assumption is that it can change - if it can't then I don't need $lang in
> the cache.

  It can't change during a single request.

> That said, let me try to come up with a better way of doing this all.

  Okay. :-)
(In reply to comment #4)
> (In reply to comment #3)
> > My assumption is that it can change - if it can't then I don't need $lang in
> > the cache.
> 
>   It can't change during a single request.

Yeah, but I want to reuse templates :)

> 
> > That said, let me try to come up with a better way of doing this all.
> 
>   Okay. :-)

I have something; I'll tidy it up and see where it ends up
Attached patch Patch (obsolete) — Splinter Review
OK, try this. Open to these being in Bugzilla.pm vs Bugzilla::Util

perf improvements:

60 comments: 895ms => 526ms
3 comments: 235ms => 190ms

Note that some of the perf improvement is because field-descs is slow (the FOREACH that merges in the field descs ends up being slow because TT's iterators are slow). I have some other patches to fix this, but it still ends up being a big win over starting a new template object multiple times for each comment.

I don't have to worry about using non-default languages here - this code (and the code its replacing) uses template_inner so won't work if we were in a subtemplate in the first place, which means that the language must be "".
Attachment #383235 - Attachment is obsolete: true
Attachment #383940 - Flags: review?(mkanat)
Attachment #383940 - Flags: review?(mkanat) → review-
Comment on attachment 383940 [details] [diff] [review]
Patch

Yeah, this is definitely too complex. Let's keep it in Bugzilla::Util and just cache the returned results somewhere in request_cache?
And since we're adding so much stuff to request_cache, we should probably take this opportunity (in some other bug) to give it some better syntax, like:

get: Bugzilla->request_cache('name');
set: Bugzilla->request_cache('name', $value)
set for non-Bugzilla.pm: Bugzilla->request_cache->set('name', $value, 'Bugzilla::SomeClass')

Then the question is what would be the best "get for non-Bugzilla.pm class" syntax. But this all belongs in another bug anyway.
I can easily move it to Bugzilla::Util, but I don't want to do a caching solution - getting all the data in one go is much quicker.
Attached patch v2 (obsolete) — Splinter Review
Try this. Note that get_* does *NOT* html filter the results; the old code has messages.html.tmpl html filter the result, then message.txt.tmpl undid that with the txt filter, then Template.pm used html_quote.

The sub returns the unfiltered result and lets the user filter it, which is much simpler.
Attachment #383940 - Attachment is obsolete: true
Attachment #384047 - Flags: review?(mkanat)
Attached patch v2.1 (obsolete) — Splinter Review
Oops, extra hunk snuck in there
Attachment #384047 - Attachment is obsolete: true
Attachment #384048 - Flags: review?(mkanat)
Attachment #384047 - Flags: review?(mkanat)
Comment on attachment 384048 [details] [diff] [review]
v2.1

>+++ template/en/default/global/get-descs.none.tmpl	19 Jun 2009 03:19:14 -0000

>+  # The Initial Developer of the Original Code is Netscape Communications
>+  # Corporation. Portions created by Netscape are
>+  # Copyright (C) 1998 Netscape Communications Corporation. All
>+  # Rights Reserved.

This block is wrong AFAIK.


>+[% PROCESS "global/field-descs.none.tmpl" %]
>+[% PROCESS global/variables.none.tmpl %]

field-descs.none.tmpl already calls variables.none.tmpl, so you don't need to call variables.none.tmpl here.
(In reply to comment #12)
> (From update of attachment 384048 [details] [diff] [review])
> >+++ template/en/default/global/get-descs.none.tmpl	19 Jun 2009 03:19:14 -0000

> >+[% PROCESS "global/field-descs.none.tmpl" %]
> >+[% PROCESS global/variables.none.tmpl %]
> 
> field-descs.none.tmpl already calls variables.none.tmpl, so you don't need to
> call variables.none.tmpl here.

Thats an implementation detail... (TT needs [% REQUIRE %], but thats a separate issue)
It's a guarantee, not an implementation detail--we replace variables.none with fields.html everywhere when we switch to using fields.html.
Blocks: 394438
Attached patch v3 (obsolete) — Splinter Review
18.9->12.1 seconds for a bug with three comments (40% improvement)
83.7->46.9 for 58 comments (44% improvement)

(times are for 100 runs)
Attachment #384048 - Attachment is obsolete: true
Attachment #394714 - Flags: review?(mkanat)
Attachment #384048 - Flags: review?(mkanat)
Duplicate of this bug: 518397
Comment on attachment 394714 [details] [diff] [review]
v3

Unfortunately bitrotten due to bug 512623. status_descs and resolution_descs no longer exist.
Attachment #394714 - Flags: review?(mkanat) → review-
(In reply to comment #17)
> (From update of attachment 394714 [details] [diff] [review])
> Unfortunately bitrotten due to bug 512623. status_descs and resolution_descs no
> longer exist.

Hmm. That sucks, because not only is there no nice way to do what I did before, I have to deal with the localisation issues too.

(Note to self - while my previous patch mostly applies to 3.4, it removed the HTML filtering that was previously there so still needs changes)
Attached patch v4 - fix & update (obsolete) — Splinter Review
Actually, this works just fine. Perf improvement with 58 comments - average from 0.82s to 0.45

the 'API' is changed - the previous code returned HTML quoted values, but this requires the caller to quote if required.

(The html quoting getting the bug term is correct since $text is already quoted)
Attachment #394714 - Attachment is obsolete: true
Attachment #412546 - Flags: review?(mkanat)
Attached patch v4 - 3.4 version (obsolete) — Splinter Review
3.4 version. I kept the same API as HEAD even though filling in the hash is slightly odder.

Perf (different bug to before with fewer comments, so can't compare to the previous results on HEAD) - 0.75s => 0.43s
Attachment #412550 - Flags: review?(mkanat)
Comment on attachment 412546 [details] [diff] [review]
v4 - fix & update

Hmm. My only concern is that the display_value MACRO could be customized (or modified by an extension?)--something that I have in fact actually done for a client recently. I wonder, is there a way to get that MACRO out of TT?
Comment on attachment 412546 [details] [diff] [review]
v4 - fix & update

Oh, also, get_term and get_value_desc will need POD.

Also, I'd prefer to combine them into one method, so that all we have to do to add is add new variables to get-descs in the future if we want more template information. In fact, they could even possibly return a hash? Not sure, though. (If you get the MACRO thing working, then this would be all determined by whatever works best with that solution.)
Attachment #412546 - Flags: review?(mkanat) → review-
Why not adding field-descs.none.tmpl only once into global/initialize.none.tmpl, and removing all other occurences?
(In reply to comment #23)
> Why not adding field-descs.none.tmpl only once into
> global/initialize.none.tmpl, and removing all other occurences?

  You know, that's a pretty interesting thought, but it would require some pretty careful testing and exploration. Feel free to file another bug for it, though.
(In reply to comment #24)
> Feel free to file another bug for it, though.

Why not doing this exploration in this bug? If we implement my suggestion, this would mean backing out bbaetz's patch later.
(In reply to comment #25)
> Why not doing this exploration in this bug? If we implement my suggestion, this
> would mean backing out bbaetz's patch later.

  because bbaetz's patch needs to go in on 3.4 and is a high-priority item, whereas the field-descs thing is a broader architectural change that can wait.
Target Milestone: --- → Bugzilla 3.4
Comment on attachment 412550 [details] [diff] [review]
v4 - 3.4 version

This is r- only because I'd like to see it conform to the same API as whatever we pick for HEAD.

Also, let's call it get-vars.none.tmpl since we might add new variables to it that aren't descriptions (though I can't currently imagine what that would be).
Attachment #412550 - Flags: review?(mkanat) → review-
(In reply to comment #21)
> (From update of attachment 412546 [details] [diff] [review])
> Hmm. My only concern is that the display_value MACRO could be customized (or
> modified by an extension?)--something that I have in fact actually done for a
> client recently. I wonder, is there a way to get that MACRO out of TT?

MACROs are processed at compile time (ie checksetup.pl). They don't exist later on.

What were you modifying it to do?

(In reply to comment #23)
> Why not adding field-descs.none.tmpl only once into
> global/initialize.none.tmpl, and removing all other occurences?

That would actually be worse perf-wise - then it would be run for *EVERY* ->process, even the ones we don't want need it for. (initialize is loaded through PRE_PROCESS, which just means its run every time ->process is called, at run time, including for each $template_inner->process
(In reply to comment #28)
> That would actually be worse perf-wise - then it would be run for *EVERY*
> ->process, even the ones we don't want need it for. (initialize is loaded
> through PRE_PROCESS, which just means its run every time ->process is called,
> at run time, including for each $template_inner->process

  Interesting. I wonder if there would be a way to process and load the template content into the constants namespace, during create(). (That would cause problems during checksetup, but perhaps they could be worked around.)
(In reply to comment #28)
> What were you modifying it to do?

  It returned different data based what user was logged in currently.

  Also, looking at bug 434381, we may want to customize it for certain sorts of things to show "${field}.description" if there's nothing specified, instead of ${field}.name.

  There's lots of things we could modify it to do, I'm sure.
(In reply to comment #30)
>   There's lots of things we could modify it to do, I'm sure.

  Perhaps the solution there is to make it a filter, FILTER value(field), and then implement its code in Perl instead of as a macro. (field could be either a field name or a Bugzilla::Field object).
Also, have you thought about the possibility of passing the $context object to quoteUrls, and using {terms} from the stash in the $context object instead of get_term, if $context is defined? That might be a quick and good fix that could even be backported to 3.4 pretty easily.
(In reply to comment #31)
>   Perhaps the solution there is to make it a filter, FILTER value(field), and
> then implement its code in Perl instead of as a macro. (field could be either a
> field name or a Bugzilla::Field object).

  Or we might have to put display_value in VARIABLES, so that we can get the $context object to get {value_descs} out of. Then if {value_descs} wasn't defined in the stash, we could just have $context process field-descs, which seems pretty reasonable to me.
Summary: quoteUrls needs to cache 'bug' term → Calling get_text in quoteUrls over and over is very slow (show_bug.cgi page with many comments is slow)
I have a new patch for HEAD only that requires the patch on bug 508823.
Depends on: 508823
Attached patch template_var, HEAD, v1 (obsolete) — Splinter Review
Here's a new version for HEAD. I did some profiling, and it seems that the actual slow part of loading templates over and over was field_descs (the hash itself). So I moved things around so that field_descs is always only loaded once per request per language, no matter how many templates are processed.
Assignee: bbaetz → mkanat
Attachment #426136 - Flags: review?(LpSolit)
Attached patch template_var, HEAD, v2 (obsolete) — Splinter Review
While I was working on this anyhow, I figured I'd switch get_text('term', { term => 'bug' }) to using template_var too. Also, there was a comment that I didn't complete writing in the previous patch, which I've completed now.

My profiling doesn't show any really significant speed-up by converting the other get_text calls at this point (and they'd be pretty tough to convert). There might be other stuff to improve about show_bug, but as far as showing comments, just improving the loading of field_descs seems to have been the biggest part of the speed problem.
Attachment #426136 - Attachment is obsolete: true
Attachment #426149 - Flags: review?(LpSolit)
Attachment #426136 - Flags: review?(LpSolit)
Attachment #412546 - Attachment is obsolete: true
Attachment #412550 - Attachment is obsolete: true
Comment on attachment 426149 [details] [diff] [review]
template_var, HEAD, v2

>=== modified file 'template/en/default/global/field-descs.none.tmpl'

>-[% field_descs = { "[Bug creation]"          => "[$terms.Bug creation]",

Why removing %field_descs from this template? That's in 90% of cases the single reason why a template calls
[% PROCESS global/field-descs.none.tmpl %]. IMO, it would be better to remove all these calls from all templates, and simply call field-descs.none.tmpl from Bugzilla::Util::template_var() if not already in Bugzilla->request_cache. For other hashes in field-descs.none.tmpl, we could call them the exact same way as you do here.
(In reply to comment #37)
>  IMO, it would be better to remove
> all these calls from all templates, and simply call field-descs.none.tmpl from
> Bugzilla::Util::template_var() if not already in Bugzilla->request_cache.

  Yeah, I understand that, but that would have been a huge patch, where I would have had to look at every single template in Bugzilla, and this patch was small.

> For
> other hashes in field-descs.none.tmpl, we could call them the exact same way as
> you do here.

  It's not necessary to add the function for every single variable, because the rest of them aren't slow.
Attached patch template_var, HEAD, v3 (obsolete) — Splinter Review
Here's a version that doesn't involve moving field_descs into vars.none, thanks to a good suggestion from LpSolit on IRC.
Attachment #426149 - Attachment is obsolete: true
Attachment #431638 - Flags: review?(LpSolit)
Attachment #426149 - Flags: review?(LpSolit)
Comment on attachment 431638 [details] [diff] [review]
template_var, HEAD, v3

>=== added file 'template/en/default/global/vars.none.tmpl'

As discussed on IRC, we don't need this template for now. template_var() should call field-descs.none.tmpl directly.
Attachment #431638 - Flags: review?(LpSolit) → review-
Attached patch template_var, v4Splinter Review
Okay, I switched it to using field-descs! I also made it cache all of %vars so that we don't re-process field-descs on every call for a different variable. I also fixed the Voting extension, which would have broken Bugzilla via infinite loop. Oh, and I also moved the in_template_var block to the end of field-descs.none, which will make it easier to add other items in field_descs to "vars" if we have to, later.

We can remove field-descs.none.tmpl from templates that no longer need it, in a separate bug.
Attachment #431638 - Attachment is obsolete: true
Attachment #432313 - Flags: review?(LpSolit)
  By the way, with my patch applied, rendering the content of bug 18574 takes an average of 3.7 seconds on my local machine, as opposed to 7 seconds without the patch. So that's a pretty big win.
Comment on attachment 432313 [details] [diff] [review]
template_var, v4

Wow, great! On my own testing with 65 comments, I also see a ratio of 2 with your patch applied. 3.2s -> 1.6s r=LpSolit
Attachment #432313 - Flags: review?(LpSolit) → review+
Assuming the patch still works with 3.4 as is.
Flags: approval3.6+
Flags: approval3.4+
Flags: approval+
  Hmm. We can't take this exact patch on 3.4. It will break extensions that use the field-descs-end hook, which is probably a fair number of extensions.

  For 3.6, LpSolit's argument was that this is a big and important enough improvement that it's OK to break extensions, because we haven't released yet, and I think that's a strong enough argument to break extensions.
Flags: approval3.4+
  Oh wait. Also, this patch requires all of the dependencies, which were significant rearchitectures. So I need a different patch for 3.6.
Flags: approval3.6+
Okay, here's the 3.6 backport. The only differences are that "votes" is still a field in 3.6 in field-descs, and that I have to use Bugzilla->request_cache->{language} instead of $template->context->{bz_language}.
Attachment #432323 - Flags: review?(LpSolit)
Attached patch Cache get_text (3.4) (obsolete) — Splinter Review
Here's an ugly hack for 3.4 that at least makes the problem a little better for that branch. (On my profiles, field-descs.none.tmpl goes from 5 seconds to 2 seconds.)
Attachment #432325 - Flags: review?(LpSolit)
Attached patch 3.4 patch, v2Splinter Review
Actually, it also looks like we never got rid of field-descs.none.tmpl in format_comment.txt.tmpl on the 3.4 branch, so I'm doing that here, too. This makes a huge difference.
Attachment #432325 - Attachment is obsolete: true
Attachment #432326 - Flags: review?(LpSolit)
Attachment #432325 - Flags: review?(LpSolit)
Comment on attachment 432323 [details] [diff] [review]
template_var, v4 (3.6)

The ratio of ~2 is still here. Cool! r=LpSolit
Attachment #432323 - Flags: review?(LpSolit) → review+
Flags: approval3.6+
Attachment #432326 - Flags: review?(LpSolit) → review+
Comment on attachment 432326 [details] [diff] [review]
3.4 patch, v2

The ratio for 3.4 is 3, woot! r=LpSolit
Flags: approval3.4+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Template.pm
modified Bugzilla/Util.pm
modified extensions/Voting/template/en/default/hook/global/field-descs-end.none.tmpl
modified template/en/default/global/field-descs.none.tmpl

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Template.pm
modified Bugzilla/Util.pm
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7032.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/                         
modified Bugzilla/Template.pm
modified template/en/default/bug/format_comment.txt.tmpl
Committed revision 6747.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.