912531 - Improve performance of process_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frédéric Buclin]

I found several places which consume time when editing bugs (there is the well known problem of the generation of bugmails but there are also some other places). I will attach a patch soon which improves performance a bit.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

What my patch does:

- preload bugs blocking/blocked by the current bug as well as duplicates, as they will be displayed in the UI. It's faster to check visibility for them all at once than one by one as we do currently;

- reuse the same user cache for all bugmails so that visibility checks do not need to be redone. The difference is mainly noticable when the dependency tree is involved (bugs added/removed or notifications when a blocker is resolved/reopened);

- set_comment_is_private() was terribly slow for users in the insidergroup, because the number of loops was O(n^2) where n = #comments. Now it's O(n). For a bug with 100 comments, this means 100 less loops.

- isopened() is now cached as it's used a lot, especially in get_bug_link().

- remove obsolete calls to Bugzilla->dbh in get_*_link().

- replace |FILTER bug_link()| by direct calls to bug/link.html.tmpl. This makes a huge difference, because TT is slow in recursions.

- no need to call bug.check_can_change_field('longdesc', ...) again and again for each comment. So we now do this check only once.

Comment 2

[:glob ✱]

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

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

::: Bugzilla/BugMail.pm
@@ +178,5 @@
>          ? Bugzilla::Bug->new_from_list([uniq @referenced_bug_ids])
>          : [];
>  
> +    # Make sure $user_cache has every user in it so far referenced
> +    $user_cache->{$_} ||= new Bugzilla::User($_) foreach (keys %recipients);

there's no need to create your own user_cache, just pass |cache => 1| to new() and let Bugzilla::Object cache for you.

Comment 3

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #2)
> there's no need to create your own user_cache, just pass |cache => 1| to
> new() and let Bugzilla::Object cache for you.

There is a need for it. The CC list and watchers are not cached.

Comment 4

[:glob ✱]

(In reply to Frédéric Buclin from comment #3)
> There is a need for it. The CC list and watchers are not cached.

that's an issue; can you please fix that code to use the object-cache as part of this patch.

Comment 5

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #4)
> (In reply to Frédéric Buclin from comment #3)
> > There is a need for it. The CC list and watchers are not cached.
> 
> that's an issue

That's not a bug, thats's a limitation of the way objects are cached. $bug->cc_users calls new_from_list(), which doesn't cache objects, see bug 819420. So that's out of the scope of this bug.

Comment 6

[:glob ✱]

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

i'll take over this bug

Comment 7

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #6)
> i'll take over this bug

Why not reviewing my existing patch first instead of bindly removing my request for review?? It sounds like you simply decided to ignore my work. The fact that I'm no longer contributing to the project is not a reason to do so.

Comment 8

[:glob ✱]

(In reply to Frédéric Buclin from comment #7)
> (In reply to Byron Jones ‹:glob› from comment #6)
> > i'll take over this bug
> 
> Why not reviewing my existing patch first instead of bindly removing my
> request for review?? It sounds like you simply decided to ignore my work.
> The fact that I'm no longer contributing to the project is not a reason to
> do so.

hi frederic,

this patch needs an owner, and with you gone i'm taking over that responsibility.

i never said i planned to ignore your work, and this isn't the case at all.  however as per my comments on this bug i don't agree with the creation of another cache, and i will address that issue in the next revision of this patch.

Comment 9

[Frédéric Buclin]

ping? any progress? There are some obvious perf wins besides the changes I made in the BugMail code. Those would be good to have for 5.0.

Comment 10

[Frédéric Buclin]

Attachment: patch, v2

Removing the user cache, keeping everything else.

Comment 11

[Frédéric Buclin]

Attachment: patch, v3

I did more optimisations. I also realized that there were new performance regressions due to new features in 5.0, aka comment tagging and multiple bug aliases:

- process_bug.cgi didn't preload the bug it was going to display. show_bug.cgi does. This makes a huge difference.

- _preload_referenced_bugs() was suboptimal as it was calling the full quoteUrls() subroutine instead of simply running needed regexps to extract bug IDs from comments. I implemented _extract_bug_ids() to do exactly that.

- set_comment_is_private() is now O(n) instead of O(n^2), where n = #comments. This was the slowest method executed by process_bug.cgi.

- attachments() now caches attachment objects so that they can be reused by comments later. This was by far the slowest part of bug/format_comment.txt.tmpl (more than 60% of the time spent in this template).

- isopened() now caches its data as it's used a lot, especially in get_bug_link().

- _bugs_in_order() is used to get the list of bugs blocking and blocked by the current bug. But the UI displays their aliases instead of their bug ID when available. Till Bugzilla 4.4, aliases were automatically loaded when calling Bugzilla::Bug->new(), but now that they are in their own DB table, each bug was triggering its own DB call to get them. This was a performance regression. So now they are all loaded at once to avoid subsequent DB calls.

- ValidateDependencies() was creating one new SQL query per blocker, even if there are no changes to the dependency list. Now only 2 queries are prepared are reused.

- Bugzilla::Comment->preload() was loading all comment tags at once to save some time. That's a good idea! The problem is that it didn't set $comment->{tags} = [] for comments with no tags, and each comment was querying the DB again to get them. I fixed this regression.

- Each comment was calling Bugzilla->params->{'comment_taggers_group'} and Bugzilla->params->{'collapsed_comment_tags'} several times. Despite Bugzilla->params is pretty fast, this represented 1000+ calls in my testing, which was by far the most consuming part of all calls made to Bugzilla->params. I now store their value using |state $foo|, which is way faster.

- bug.check_can_change_field('longdesc', ...) was called twice per comment. It's now called only once per *bug*!

- |FILTER bug_link| is definitely slow, because TT is slow to do recursions. Way too slow to be used in loops. I replaced it by a direct call to bug/link.html.tmpl, which is much faster. This is also why I objected to fix bug 115796. It would be a *huge* performance regression.


Now some numbers. I tested my patch against a bug with 150+ dependencies, 180+ comments, 50+ attachments and 10+ users in the CC field:

Without patch (process_bug.cgi):

 3.76s (of 4.52s), executing 1023599 statements and 438831 subroutine calls

With patch (process_bug.cgi):

 3.21s (of 3.85s), executing 863949 statements and 330971 subroutine calls

This is a 15-25% win.


Without patch:

 bug/comments.html.tmpl     : 913ms
 get_bug_link()             : 434ms
 set_comment_is_private()   : 263ms
 body_full()                : 110ms
 bug/format_comment.txt.tmpl:  42ms

With patch:

 bug/comments.html.tmpl     : 759ms
 get_bug_link()             :  84ms
 set_comment_is_private()   :  19ms
 body_full()                :  87ms
 bug/format_comment.txt.tmpl:  19ms

Comment 12

[Frédéric Buclin]

dkl: any progress? That's really something I would like to see in 5.0.

Comment 13

[Frédéric Buclin]

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

This patch has been split into 4 independent patches, see the dependency list of this bug. This should make reviewer's life easier.

Comment 14

[David Lawrence [:dkl]]

Progress has been made on the dependent bugs. Please close this 'tracking' bug when the others are committed.

dkl

Comment 15

[Frédéric Buclin]

All blockers fixed.