552167 - show_bug.cgi loads flagtypes from the database for each attachment

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Max Kanat-Alexander]

I was doing some profiling of show_bug.cgi using a local copy of the bmo database, and I was testing against bug 18574. For some reason, in the DBI profile, the following SQL is run 63 times and takes up .23 seconds:

SELECT flagtypes.id,flagtypes.name,flagtypes.description,flagtypes.cc_list,flagtypes.target_type,flagtypes.sortkey,flagtypes.is_active,flagtypes.is_requestable,flagtypes.is_requesteeble,flagtypes.is_multiplicable,flagtypes.grant_group_id,flagtypes.request_group_id FROM flagtypes WHERE  id IN (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)   ORDER BY flagtypes.sortkey, flagtypes.name

This is the largest SQL time-taker in show_bug.cgi.

Comment 1

[Max Kanat-Alexander]

  Ah, I have found the culprit. Bugzilla::Attachment calls Bugzilla::Flag->_flag_types once for each attachment. This also results in the #1 time-taker in an NYTProf profile, which is the "sort" at the end of Bugzilla::Flag->_flagtypes. (Takes .5 seconds with 63 attachments.)

  Probably the solution is to cache Bugzilla::Attachment->_flag_types for the bug's component, in request_cache?

Comment 2

[Max Kanat-Alexander]

Attachment: v1

For a bug with 63 attachments, this could shave almost an entire second off of loading time.

Comment 3

[Frédéric Buclin]

Comment on attachment 432322 [details] [diff] [review]
v1

You mixed your patch with the template_var's one.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

Here's the right patch. :-)

Comment 5

[Frédéric Buclin]

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

>-    $self->{flag_types} = Bugzilla::Flag->_flag_types($vars);
>+    $cache->{$component_id} = Bugzilla::Flag->_flag_types($vars);

Bugzilla::Flag->_flag_types() returns data which is unique for each attachment. It looks for valid flag types, but also for existing flags set on the attachment, so you cannot share it between attachments.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Bugzilla::Flag->_flag_types() returns data which is unique for each attachment.
> It looks for valid flag types, but also for existing flags set on the
> attachment, so you cannot share it between attachments.

  Okay. Then why is it being called on show_bug.cgi, a situation in which flags aren't editable?

Comment 7

[Frédéric Buclin]

Attachment: patch, v3

Could you test that, please?

Comment 8

[Max Kanat-Alexander]

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

This fixes the DB problem.

However, the "sort" that happens at the end of _flag_types is still the largest time-taker for Perl code in show_bug.cgi, when there are a lot of attachments.

Comment 9

[Max Kanat-Alexander]

  I think caching them sorted and then refactoring the code a bit to use an array instead of a hash would solve the sorting problem.

Comment 10

[Frédéric Buclin]

Attachment: patch, v4

This removes the problem with sort(). In fact, all I need to do is to return $flag_types, which is already sorted thanks to FlagType->match(). And because %flagtypes contains a list of objects, when I populate this hash, I in fact populate objects from $flag_types. Data::Dumper::Dumper() says they are perfectly identical.

Comment 11

[Frédéric Buclin]

Is the perf win significant enough to take the patch for 3.4.7?

Comment 12

[Max Kanat-Alexander]

Comment on attachment 435914 [details] [diff] [review]
patch, v4

Awesome! :-)

Comment 13

[Max Kanat-Alexander]

  I'll let you decide whether you think it's safe to take this for 3.6.

Comment 14

[Max Kanat-Alexander]

(In reply to comment #11)
> Is the perf win significant enough to take the patch for 3.4.7?

  No, I don't think so.

Comment 15

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Flag.pm
Committed revision 7106.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/Flag.pm
Committed revision 7065.