322848 - Extremely slow performance with a lot of products (~1800) and security groups for each

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Christoph Müller]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de-DE; rv:1.7.12) Gecko/20051010 Firefox/1.0.7 (Ubuntu package 1.0.7)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; de-DE; rv:1.7.12) Gecko/20051010 Firefox/1.0.7 (Ubuntu package 1.0.7)

I've ~1800 products in Bugzilla. Every product has its own security group. So i've ~1800 security groups each assigned to one of the products. In each product only members of the assigned group can enter and edit bugs. MemberControl and OtherControl is mandatory/mandatory. Our employees are via inheritance in all of the 1800 groups, so see all 1800 products and can enter bugs.
When you are in all 1800 groups, Bugzilla gets extremly slow on nearly every page, that lists products. E.g. show_bug.cgi or enter_bug.cgi. On our server hardware (Dual Xeon machine with 2 GB ram) it takes so long, that the browser stops loading the show_bug.cgi page. 
After playing around with the code, i can say, that it is the piece of code that checks to which products the current user has access, that slowes down the whole thing. The code executes 1800 times (for every product) a select statement that takes itself on our server ~one second. That are theoretically 30 minutes, but it never runs so long, cause the browser stops loading the page long before. For testing I simply commented out the CanEnterProduct call in the function 'choices' of Bugzilla/Bug.pm. With this, the performance is tolerable, but not ok.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Frédéric Buclin]

We should try to improve these routines:

User::get_enterable_products()
User::get_selectable_products()
Product::get_all_products()

by decreasing the number of SQL calls, and maybe have a Product::get_list(@prod_ids) which works the same way as Attachment::get_list(), i.e. get all data in *one* SQL query. But Product::group_controls() would still be executed once per product, as we probably don't want to look at groups by default when creating a product object.

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> We should try to improve these routines:
> 
> User::get_enterable_products()
> User::get_selectable_products()
> Product::get_all_products()

  Okay. But before we start optimizing, we need some actual profiling data from DProf on these things. I know how to optimize them, I just did something similar for another application.

Comment 3

[Christoph Müller]

Attachment: Performance optimization for a lot of products, patch for 2.20

Meanwhile I've done two optimizations. First I could reduce the number of products from 1800 to 850. But thats not the great thing.
Second, I modified the way in which is checked if a user can enter bugs in a product. With this modification I've got a performance improvement of 8000% when opening show_bug.cgi to view a bug. It sounds unreal, but it is true! I checked it with the HiRes-Timer of Perl. With the original Bugzilla code and 850 products and a user that is in all 850 product groups it took ~214 seconds only to check in which products the current user can enter bugs. Now with this modification it takes ~2.5 seconds. I checked as good as I could, that the code does the same thing as before. I tested different ways of doing it, but this was the one that performed the best. One thing I tried was reading all group ids, in which the user needs to be in to enter bugs into a product, from the DB and checked for each if the user is a member of it by looping through the group ids in which the current user is a member. That was also a big performance boost and has done the job in ~3 seconds. And other way i tried was to select the group names via an inner join instead of the group ids and checking if the group name is a key in the hash of groups in which the user is a member. That performed the best at ~2.5 seconds.

It would be great if you merge it into Bugzilla so that all users can enjoy this performance improvement. I release it under the Mozilla Public License.

Comment 4

[Frédéric Buclin]

Comment on attachment 208665 [details] [diff] [review]
Performance optimization for a lot of products, patch for 2.20

This patch applies on 2.20 and modifies the routine CanEnterProduct() in globals.pl.

Requesting review to keep track of it.

Comment 5

[Frédéric Buclin]

Note that this patch is probably not enough, see my comment 1.

Comment 6

[Christoph Müller]

(In reply to comment #4)
> This patch applies on 2.20 and modifies the routine CanEnterProduct() in
> globals.pl.

Oh, I forgot to say, that it was developed for 2.21.1. I don't know if it works for 2.20.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> Oh, I forgot to say, that it was developed for 2.21.1. I don't know if it works
> for 2.20.
> 

No! This routine no longer exists in globals.pl in 2.21, so it cannot be a patch for 2.21. It has been renamed as User::can_enter_product(). I checked, this is really code from 2.20. Maybe you posted the wrong patch?

Comment 8

[Frédéric Buclin]

hum...ok, this code has been changed 10 days after the release of 2.21.1. Mea culpa. Anyway, this patch won't apply on 2.22rc1 per my previous comment.

Comment 9

[Christoph Müller]

(In reply to comment #8)
> hum...ok, this code has been changed 10 days after the release of 2.21.1. Mea
> culpa. Anyway, this patch won't apply on 2.22rc1 per my previous comment.
> 

I see. But the code from my patch could be easily applied to the new place in User.pm in the current trunk version.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> I see. But the code from my patch could be easily applied to the new place in
> User.pm in the current trunk version.

It depends. I would like to stop all the SQL queries here and there and move to something more object oriented. And instead of buidling new objects all over the code, we should probably find a way to store existing ones in some container so that if we require the same object again later in the code, we can look into the container instead of building the object again.

At least I hope that's the direction mkanat will take, especially that we now have Bugzilla::Products and friends. Max, something tells me that we will have tons of cleanup to do before 2.24. ;)

Comment 11

[Christoph Müller]

> It depends. I would like to stop all the SQL queries here and there and move to
> something more object oriented. And instead of buidling new objects all over
> the code, we should probably find a way to store existing ones in some
> container so that if we require the same object again later in the code, we can
> look into the container instead of building the object again.

Does that situation, that the same object is created more than one time in one round trip, occurs so often? I think a cache, that holds the objects for all users and for several round trips would be more helpful. But that will only be possible with mod_perl.

For speeding up the creation of e.g. product objects, I would provide the possibility to pass a database row to the constructor of the object instead of passing only the id or name and the object itself talks to the database again. It would be more effective to "select col1, col2, col3 from products ..." and pass the column array to the product constructor, that builds itself from that values. That way, only one db query needs to run.

Comment 12

[Max Kanat-Alexander]

The most important thing is profiling and good, maintanable architecture. That will lead to good performance. Whatever the *minimal* changes are to get excellent performance, that's what we'll do.

Comment 13

[Frédéric Buclin]

(In reply to comment #11)
> For speeding up the creation of e.g. product objects, I would provide the
> possibility to pass a database row to the constructor of the object instead of
> passing only the id or name and the object itself talks to the database again.

Right! That's what I have in mind too. We actually do it for Attachment.pm. There is no reason not to do it with product objects too. We could have a Bugzilla::Product->new_from_list() which returns an array of product objects.

Sure, 1 SQL call instead of 1800 successive ones is a huge benefit. ;)

Comment 14

[Joel Peshkin]

OK, we talked this over on IRC

We need to do the following...

1) Add a new_from_list to product.pm
2) change get_enterable_products to use the same style of single query as get_selectable_products uses
3) change get_enterable_products and get_selectable_products to use the new_from_list mechanism to get the list of product objects instead of calling the constructor a zillion times. (and cache the result)
4) Have Bug.pm's choices routine use get_enterable_products.

Comment 15

[Frédéric Buclin]

Note also that User::get_selectable_products() already takes care of user privs, so Product::new_from_list() doesn't need to JOIN group_control_map. In other words, this constructor doesn't care about privileges, which will make it even faster.

I still plan to see it on the 2.22 branch; it's not invasive and has huge benefits.

Comment 16

[Max Kanat-Alexander]

Attachment: v1

Okay, here's a somewhat-tested version of this patch. I checked all the various "you can't enter a bug into this product" conditions on individual products on my landfill installation.

You'll notice that because we now use enterable_products, we can't determine *why* we're denying entry to a product. If anybody has a good, simple way to work around this, please feel free to suggest it. However, I've modified the error message to include all the possible reasons a user wouldn't be able to enter a bug.

This also adds the Bugzilla::Product->new_from_list method, which is fairly simple. After this patch is committed, everywhere that wants to use a list of products in Bugzilla should start using this function. I didn't do that in this patch because (a) that's not the subject of this bug and (b) this patch will also go into 2.22.

Comment 17

[Max Kanat-Alexander]

Attachment: v2 (also include get_selectable_products)

I had forgotten to also fix get_selectable_products.

Bug::choices actually doesn't need to be fixed for performance at this point, since it just calls can_enter_product which is now quite fast. When we remove @::enterable_products (in another bug) we can switch it around.

Comment 18

[Frédéric Buclin]

Comment on attachment 212711 [details] [diff] [review]
v2 (also include get_selectable_products)

>Index: Bugzilla/User.pm

>+    my $can_enter = 
>+        grep($_->name eq $product_name, @{$self->get_enterable_products});

>+    if (!$can_enter) {

AFAIK, grep() returns an array. You should write @enterable_products instead of $can_enter and then look at scalar(@enterable_products). Even if only one product is returned, I still strongly prefer using an array here.


>+        return 0 unless $warn;
>         ThrowUserError('entry_access_denied', { product => $product_name });
>     }

I'm pretty sure some parts of the code behave differently if the function returns undef or 0. To avoid any regression, I would keep 'return undef unless $warn' here.


>+    # All products which the user has "Entry" access to.
>+    my @enterable_ids =@{$dbh->selectcol_arrayref(
>+          'SELECT products.id FROM products
>+        LEFT JOIN group_control_map
>+                  ON group_control_map.product_id = products.id
>+                     AND group_control_map.entry != 0
>+                     AND group_id NOT IN (' . $self->groups_as_string . ')
>+           WHERE group_id IS NULL
>+                 AND products.disallownew = 0')};

Nit: indentation of WHERE! :-p Moreover, I would move 'FROM products' on its own line.


>+    @enterable_ids = @{$dbh->selectcol_arrayref(
>+            'SELECT products.id FROM products
>+        INNER JOIN components ON components.product_id = products.id
>+        INNER JOIN versions ON versions.product_id = products.id
>+             WHERE products.id IN (' . (join(',', @enterable_ids) || '-1') .
>+        ')' . $dbh->sql_group_by('products.id') . '
>+           HAVING COUNT(components.id) != 0')};

Three things:
- First, I don't see why you add 'GROUP BY products.id HAVING COUNT(...)'. You use an INNER JOIN with components, so COUNT() cannot be 0.
- Secondly, if @enterable_ids was initially empty, I would not do this SQL query at all, instead of looking for -1.
- The third point is that you use a reference to the array when calling new_from_list() below, so converting selectcol_arrayref to an array to pass it by reference again later doesn't make sense to me.

>+    $self->{enterable_products} = 
>+        Bugzilla::Product->new_from_list(\@enterable_ids);

See my third comment above.



>Index: template/en/default/global/user-error.html.tmpl

>+    You cannot enter [% terms.abug %] into the 
>+    <em>[% product FILTER html %]</em> product. That means either:
>+    (a) The product does not exist, (b) you are not authorized
>+    to enter [% terms.abug %] into it, (c) it has no components,
>+    (d) it has no versions, or (e) it has been disabled for 
>+    [% terms.bug %] entry.

Doing no distinction anymore about the reason we reject the product is a big regression IMO. We wrote several patches to get it working fine.


But globally, I think we are doing the right thing here.

Comment 19

[Joel Peshkin]

Comment on attachment 212711 [details] [diff] [review]
v2 (also include get_selectable_products)

Logic looks right.

Fix the items that Lpsolit points out.  (especialy the HAVING) 

In order to solve the last item, let's just make can_enter_product return a successfult response if the product is in the enterable products list, but perform the additional queries to figure out why if the product is not enterable.  This will only add a few queries if there is an error and will not slow us down at all int the pass case.

Comment 20

[Frédéric Buclin]

vladd, a perf issue is not an enhancement, but a bug.

Comment 21

[Frédéric Buclin]

mkanat, have you an updated patch for this bug?

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> mkanat, have you an updated patch for this bug?

  Not yet, but if you'd like to see one soon I could get you one soon (like, perhaps this week).

Comment 23

[Vlad Dascalu]

(In reply to comment #20)
> vladd, a perf issue is not an enhancement, but a bug.

Then we should rewrite Bugzilla in ASM. ;)

Comment 24

[Max Kanat-Alexander]

Attachment: v3

Okay, I did everything you guys asked for. I removed the HAVING, and I kept all of the old functionality by following joel's suggestion.

Remember that you don't have to worry about checking off the SQL, joel did look at it and say it was good.

I tested this pretty thorougly, all the cases perform correctly.

Comment 25

[Max Kanat-Alexander]

Attachment: v3.1

I had left in some debugging code in the previous patch.

Comment 26

[Frédéric Buclin]

Comment on attachment 230014 [details] [diff] [review]
v3.1


>+    if (!$can_enter) {

Nit: instead of this big IF block, you should return 1 now. This way you avoid this block completely. That's the kind of nice cleanup kiko taught to me when I joined Bugzilla. ;)


>+        # Or if we just don't have access, this is where we'll get to.
>+        else {
>+            ThrowUserError('entry_access_denied', {product => $product->name});
>+        }

That's the main reason of my r-. If a product I have no access to is e.g. closed for new bugs, the error message will say that the product is closed for new bugs instead of telling me I'm not allowed to access the product. This error must be returned earlier.


> sub get_enterable_products {

>+     my @enterable_ids =@{$dbh->selectcol_arrayref(
>+           'SELECT products.id FROM products

Nit: Probably a DISTINCT would be useful here.


>+     @enterable_ids = @{$dbh->selectcol_arrayref(
>+             'SELECT products.id FROM products
>+         INNER JOIN components ON components.product_id = products.id
>+         INNER JOIN versions ON versions.product_id = products.id
>+              WHERE products.id IN (' . (join(',', @enterable_ids) || '-1') .
>+         ')' . $dbh->sql_group_by('products.id')) || []};

Several things, which are also part of my r-:
1. Use DISTINCT instead of GROUP BY.
2. Return earlier if you already know that @enterable_ids is empty from the previous SQL query. This avoids this ugly "|| -1" hack which I really dislike. Or use a IF block.

Else the patch looks promising. Much less SQL calls than before. :)

Comment 27

[Max Kanat-Alexander]

Attachment: v4

Okay, I fixed all the comments above.

Comment 28

[Frédéric Buclin]

Comment on attachment 230062 [details] [diff] [review]
v4

>Index: Bugzilla/User.pm

>+    die "can_enter_product reached an unreachable location.";

ThrowCodeError() would be much better. 1. It's localisable, and 2. you don't display the path to your installation. Theoretically, we can never come here, though. We should maybe open a separate bug to replace this die().

Looks good. r=LpSolit for tip only.

Comment 29

[Max Kanat-Alexander]

Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.120; previous revision: 1.119
done

Comment 30

[Christoph Müller]

I tested the current Bugzilla 3.0 rc1 with our DB with these many products and security groups. I set it up to run with mod_perl2. But the performance is still very very bad. E.g. to display the enter_bug page that lists all products, it takes ~2-3 Minutes.
I think it's cause Bugzilla still sends that big "WHERE id IN (all 1800 group ids.....)" SQL statements to the DB.

I'm convinced, that the new_from_list-methods speed up the creation of object collections, but on the whole, from the user perspective, I don't feel any speed improvement. It still feels impractically to work with with this great number of products and security groups.

Comment 31

[Max Kanat-Alexander]

Yes, I agree with you. See bug 372795. We are going to leave this bug marked as FIXED for now, though, since we checked in code that did improve performance.