878623 - Improvement in retrieving a classification name from bug

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Simon Green]

Attachment: v1 patch

This is one of the trivial things that will save about 1ms per call, but when you make the call a few thousand times (as is the case for the Bug.search RPC call), that means seconds shaved off.

Calling Bugzilla::Bug->classification means that two calls are made to database, once to get the classification_id from the product, the second to get the classification name from it's id.

This changes that so only one call is made.

Comment 1

[:glob ✱]

Comment on attachment 757189 [details] [diff] [review]
v1 patch

while this works, it would be better to return $self->product_obj->classification->name (with a similar change to the classification_id sub).

the object cache will ensure we don't hit the database multiple times when there is more than one bug in the same product.

Comment 2

[Simon Green]

(In reply to Byron Jones ‹:glob› from comment #1)
> Comment on attachment 757189 [details] [diff] [review]
> v1 patch
> 
> while this works, it would be better to return
> $self->product_obj->classification->name (with a similar change to the
> classification_id sub).
> 
> the object cache will ensure we don't hit the database multiple times when
> there is more than one bug in the same product.

Unless I am missing something, there is no object caching of products in Bugzilla. $bug->product_obj(<id>) will do one select call in Bugzilla::Object::_init for each bug. I can't see anywhere in the code where this is cached.

Comment 3

[Simon Green]

Attachment: v2 patch

Comment 4

[:glob ✱]

(In reply to Simon Green from comment #2)
> Unless I am missing something, there is no object caching of products in
> Bugzilla. $bug->product_obj(<id>)

> $self->{product_obj} ||=
>     new Bugzilla::Product({ id => $self->{product_id}, cache => 1 });

note "cache => 1".  the object cache was added via bug 811280.

>-    return $self->{classification} if exists $self->{classification};
>-    return '' if $self->{error};
>-    ($self->{classification}) = Bugzilla->dbh->selectrow_array(
>-        'SELECT name FROM classifications WHERE id = ?',
>-        undef, $self->classification_id);
>+    $self->{classification} //= $self->product_obj->classification->name;

are you sure it's ok to no longer return an empty string if {error} is set?

Comment 5

[Frédéric Buclin]

Comment on attachment 757750 [details] [diff] [review]
v2 patch

>-    return 0 if $self->{error};

>-    return '' if $self->{error};

If the bug is not accessible, then these values must be returned. So these security checks must stay.

Comment 6

[Simon Green]

Attachment: v3 patch

Comment 7

[Simon Green]

(In reply to Frédéric Buclin from comment #5)
> If the bug is not accessible, then these values must be returned. So these
> security checks must stay.

Totally agree. I was a bit over-eager in my cutting of the old code, and didn't notice it when looking at the diff. New patch submitted.

Comment 8

[:glob ✱]

Comment on attachment 758279 [details] [diff] [review]
v3 patch

r=glob

Comment 9

[Frédéric Buclin]

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