Decrease the number of calls to the DB about flags when viewing a bug

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

({perf})

Bugzilla 3.2
Bug Flags:
approval +
approval3.2 +

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

9 years ago
Created attachment 336953 [details] [diff] [review]
patch for 3.2, v1

When viewing a bug, Bugzilla::Flag->match() is called once per bug flag type (including inactive ones!), and once per attachment (including those which are not patches), which represents a pretty large number of calls, see the URL. With my patch, this number decreases to only two calls per bug!
Attachment #336953 - Flags: review?(justdave)

Updated

9 years ago
Attachment #336953 - Flags: review+

Comment 1

9 years ago
Comment on attachment 336953 [details] [diff] [review]
patch for 3.2, v1

This is totally fine as a hack for the branch. Upstream, we should add a preload argument or something to get_attachments_by_bug (which should really be using Object::match) and something similar for FlagType or Flag, much like we did for products.

Updated

9 years ago
Target Milestone: --- → Bugzilla 3.2
(Assignee)

Comment 2

9 years ago
Created attachment 337308 [details] [diff] [review]
patch for tip, v2

I refactored the code and moved the code to get flag type data into Bugzilla::Flag::_flag_types, which is now called by both $bug->flag_types and $attachment->flag_types. To preload flag type data when calling get_attachments_by_bug(), you have to pass a true value as 2nd argument (I don't want a separate preload() function).
Attachment #337308 - Flags: review?(mkanat)

Updated

9 years ago
Attachment #336953 - Flags: review?(justdave)

Updated

9 years ago
Attachment #337308 - Flags: review?(mkanat) → review+

Comment 3

9 years ago
Comment on attachment 337308 [details] [diff] [review]
patch for tip, v2

Sure, this is fine. However, I'd like the second argument to be { preload => 1} instead of just "1", for the sake of consistency.

Also, it seems like we could be using Bugzilla::Object::match('Bugzilla::Flag', {}) or something here instead of the new _flag_types method, but perhaps for the future.

Updated

9 years ago
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 4

9 years ago
tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.147; previous revision: 1.146
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.57; previous revision: 1.56
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.259; previous revision: 1.258
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.98; previous revision: 1.97
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.53; previous revision: 1.52
done


3.2rc1:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.10; previous revision: 1.241.2.9
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

9 years ago
Created attachment 337569 [details] [diff] [review]
fix bustage on tip

I have to exclude flags related to private attachments you cannot see.

Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.59; previous revision: 1.58
done
(Assignee)

Comment 6

9 years ago
Created attachment 337570 [details] [diff] [review]
fix bustage on 3.2

Same fix, but for 3.2.

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.11; previous revision: 1.241.2.10
done
(Assignee)

Updated

9 years ago
Attachment #336953 - Attachment description: patch, v1 → patch for 3.2, v1
(Assignee)

Updated

9 years ago
Attachment #337308 - Attachment description: patch, v2 → patch for tip, v2
(Assignee)

Updated

9 years ago
Attachment #337569 - Attachment description: fix bustage → fix bustage on tip
(Assignee)

Comment 7

9 years ago
Selenium script needed to prevent error due to comment 5.
Flags: testcase?
(Assignee)

Updated

5 years ago
Flags: testcase?
You need to log in before you can comment on or make changes to this bug.