Closed Bug 595410 Opened 10 years ago Closed 9 years ago

Displaying a bug with many dependencies is slow


(Bugzilla :: Creating/Changing Bugs, defect)

Not set



Bugzilla 4.2


(Reporter: mkanat, Assigned: mkanat)



(Keywords: perf)


(1 file, 2 obsolete files)

Showing a bug with a lot of dependencies takes a very long time on Bugzilla's side.
Attached patch v1 (obsolete) — Splinter Review
In my local testing with a real bug that has several hundred dependencies, this makes show_bug 5x faster.

There are many performance improvements in this patch:

* Before, the dep list was calling bug_link on a lot of bug ids. This means that it was making a new Bug object for every bug in each list. Now it calls bug_link on bug ids.

* The dep list code was calling can_see_bug on every bug in the list individually, resulting in hundreds of SQL calls. Instead, I now call visible_bugs on all the bugs before showing the page. (I know that the preload code could be better, for other situations, but I'm not worried about those situations right now, only this situation.)

* get_bug_link itself was very slow, because it needed to do get_status and get_resolution calls to get_text. I've now made get_bug_link use a template to generate the bug link. This has many advantages beyond performance improvements, including making it easier to customize bug links, and making some text more localizable.

* To avoid processing field-descs.none.tmpl inside of bug/link.html.tmpl, I made "display_value" a function that works just like "field_descs" works now.
Assignee: create-and-change → mkanat
Attachment #474310 - Flags: review?(LpSolit)
Target Milestone: --- → Bugzilla 4.2
(In reply to comment #1)
> Now it calls bug_link on bug ids.

  That should say "Now it calls bug_link on bug objects."
Blocks: 556045
Comment on attachment 474310 [details] [diff] [review]

Sorry, but your patch doesn't apply cleanly anymore. It has been bitrotten by the "remove AUTOLOAD" bug in and by the "implement HTML bugmails" bug in
Attachment #474310 - Flags: review?(LpSolit) → review-
Attached patch v2 (obsolete) — Splinter Review
Okay, here's an updated patch that applies and works! :-)
Attachment #474310 - Attachment is obsolete: true
Attachment #496919 - Flags: review?(LpSolit)
Attachment #496919 - Flags: review?(LpSolit) → review?(glob)
Comment on attachment 496919 [details] [diff] [review]

I can probably review it this week.
Attachment #496919 - Flags: review?(LpSolit)
Comment on attachment 496919 [details] [diff] [review]

>=== modified file 'Bugzilla/'

>+sub preload {

>+    my @all_dep_ids;
>+    foreach my $bug (@$bugs) {
>+        push(@all_dep_ids, @{ $bug->blocked }, @{ $bug->dependson });
>+    }
>+    @all_dep_ids = uniq @all_dep_ids;
>+    # If we don't do this, can_see_bug will do one call per bug in
>+    # the dependency lists, during get_bug_link in Bugzilla::Template.
>+    $user->visible_bugs(\@all_dep_ids);

Could be written as:

 my %all_deps = map { $_ => 1 } (@{ $bug->blocked }, @{ $bug->dependson });
 $user->visible_bugs([keys %all_deps]);

>=== modified file 'template/en/default/bug/edit.html.tmpl'

>+    [% INCLUDE dependencies 
>+         accesskey = "b" field = bug_fields.blocked deps = bug.blocks_obj %]

accesskey is no longer in use.

>=== added file 'template/en/default/bug/link.html.tmpl'


You should also list full_url.

>+[% link_title = BLOCK %]
>+  [% display_value('bug_status', bug.bug_status) %]
>+  [%+ display_value('resolution', bug.resolution) %]
>+[% END %]

They must be FILTERed, and unfiltered fields which have been removed from bug/edit.html.tmpl must be removed from

#   Failed test '(en/default) template/en/default/bug/link.html.tmpl has unfiltered directives:
#   35: display_value('bug_status', bug.bug_status) 
#   36: display_value('resolution', bug.resolution) 
# --ERROR'
#   at t/008filter.t line 132.

#   Failed test '(en/default) template/en/default/bug/edit.html.tmpl - has extra members:
#   dep.title
#   dep.fieldname
#   " accesskey=\"$accesskey\"" IF accesskey
#   bug.${dep.fieldname}.join(', ')

Otherwise looks good, but I haven't tested it yet.
Attachment #496919 - Flags: review?(glob)
Attachment #496919 - Flags: review?(LpSolit)
Attachment #496919 - Flags: review-
Attached patch v3Splinter Review
Okay, I fixed all the last few comments, although I kept the preload code the same. (uniq is faster and I think it's a little easier to understand what's going on there if I don't use map).
Attachment #496919 - Attachment is obsolete: true
Attachment #500956 - Flags: review?(LpSolit)
Comment on attachment 496919 [details] [diff] [review]

>=== modified file 'Bugzilla/'

>-        if ($options->{use_alias} && $link_text =~ /^\d+$/ && $bug->alias) {
>-            $link_text = $bug->alias;
>-        }

>=== added file 'template/en/default/bug/link.html.tmpl'

>+  [% IF use_alias && bug.alias %]
>+    [% link_text = bug.alias %]
>+  [% END %]

Note that you dropped "$link_text =~ /^\d+$/" from the check, which means "bug 123" could be morphed into "bug foo" in comments, which would look weird, I suppose.
(In reply to comment #8)
> Note that you dropped "$link_text =~ /^\d+$/" from the check, which means "bug
> 123" could be morphed into "bug foo" in comments, which would look weird, I
> suppose.

  Oh, actually it would become just "foo". I think that might be cool, actually. (Although it would confuse people who wanted to refer to the bug in the same way in their own comments, for sure.)
Comment on attachment 500956 [details] [diff] [review]

Looks good. r=LpSolit

In my testing, get_bug_link() is 3.5x faster, which is a good improvement. :)
Attachment #500956 - Flags: review?(LpSolit) → review+
Flags: approval+
Committing to: bzr+ssh://                       
modified show_bug.cgi
modified Bugzilla/
missing Bugzilla/
modified Bugzilla/
modified Bugzilla/
modified template/en/default/
modified template/en/default/bug/edit.html.tmpl
added template/en/default/bug/link.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
Committed revision 7643.
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 624414
Blocks: 848250
You need to log in before you can comment on or make changes to this bug.