553266 - config.cgi?ctype=rdf spends most of its time loading flagtypes from the database

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

On a copy of the bmo database on my local machine, the RDF format of config.cgi takes 9 seconds to process data. A DBI profile shows that it's spending most of its time loading flag types from the database with SQL statements like this:

 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

  And then probably a fair bit of time is spent creating the flagtype objects, as well.

  Perhaps adding a parameter to Bugzilla::Product::preload() to load all the flagtypes at once would be a good idea?

Comment 1

[Frédéric Buclin]

(In reply to comment #0)
>   And then probably a fair bit of time is spent creating the flagtype objects,
> as well.

This SQL query is called by FlagType->new_from_list(), so we don't waste additional time creating objects.

It's being called once per component because the inclusion/exclusion lists make it pretty difficult to load all flagtypes objects at once and then triage them per component.

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> It's being called once per component because the inclusion/exclusion lists make
> it pretty difficult to load all flagtypes objects at once and then triage them
> per component.

  Yeah, difficult, but not impossible. You could create a data structure that mapped the *clusions to components.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

$product->flag_types now gets all flag types at once, and then triages them per component, rather than collecting each component's flag types one by one. I also cache flag types to be used across products, as I did with bug flags in bug 552167. This highly decreases the number of SQL queries, see the next paragraph. I also did another nice optimization in $component->flag_types where I collect bug flag types and attachment flag types at once, and triage them from here. This single optimization decreases the number of SQL queries by a factor 2.

On my test installation, with 17 products, 2025 components and 11 flag types, the time spent to display config.cgi in RDF format goes from 6.7s down to 3.4s. Also, the number of SQL queries goes from 4050 (twice per component) down to 39 (once per product + twice per flag type)! 1% only! :)

mkanat, could you test this patch on your local copy of bmo and tell me how big is the improvement? I'm confident about the code itself (it is in code I own).

Comment 4

[Frédéric Buclin]

For 3.6.3, we should only backport the trivial and safe optimization in Bugzilla::Component, so that we decrease the number of SQL queries by a factor 2.

Comment 5

[Max Kanat-Alexander]

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

Okay, this takes the time on my local copy of bmo from 13 seconds to 6.5 seconds. That's pretty good, considering that with "flags=0" it's 4 seconds. So now it only takes 2.5 seconds to generate and output the flags data. (And my local copy is considerably slower than the real bmo.)

I'll let you r+ this since you're the module owner.

Comment 6

[Frédéric Buclin]

OK, thanks for the test. It's a nice improvement. :) As I said in comment 4, I will only take the change in Bugzilla::Component for 3.6, which is safe and not invasive.

Comment 7

[Frédéric Buclin]

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

r=me as module owner.

Comment 8

[Frédéric Buclin]

Attachment: patch, v1.1

Optimized patch, which uses hashes instead of grep'ing through strings. mkanat, what's the improvement now?

Comment 9

[Max Kanat-Alexander]

Looks like it's a full second faster--I'm getting 5.5 seconds now. I'm going to check the NYTProf of it too, just to make sure there's nothing else important that could get better.

Comment 10

[Max Kanat-Alexander]

So just FYI, if you cache the inclusions_as_hash and exclusions_as_hash instead, you might get a faster patch. Looks like most of the time now is spent calling those:

spent   764ms making 103930 calls to Bugzilla::FlagType::inclusions_as_hash, avg 7µs/call

spent   697ms making 103930 calls to Bugzilla::FlagType::exclusions_as_hash, avg 7µs/call

Comment 11

[Frédéric Buclin]

Attachment: patch, v1.2

I'm surprised this makes a difference. Bugzilla::FlagType->inclusions already caches the data, so Bugzilla::FlagType->inclusions_as_hash is supposed to directly get the data being in the cache from inclusions(). But avoiding a useless call to inclusions() indeed saves some extra 0.1s (looks like calling a method is slow, independently of what it does).

Comment 12

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified config.cgi
modified Bugzilla/Component.pm
modified Bugzilla/FlagType.pm
modified Bugzilla/Product.pm
Committed revision 7550.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified config.cgi
modified Bugzilla/Component.pm
modified Bugzilla/FlagType.pm
modified Bugzilla/Product.pm
Committed revision 7446.

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