880093 - Cache filter_wants

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Simon Green]

BRC are hitting issues with the performance of RPC calls. One issue when retrieving a list of 4,000 bugs (with about 12 custom fields) is that Bugzilla::WebService::Utils::filters_wants is taking about 7.6 seconds to run (tested on my workstation). With this proposed change, this now takes 1.6 seconds.

I can see no reason not to cache this information, as the params value should change between calls for the same filter/prefix.

Comment 1

[Simon Green]

Attachment: v1 patch

Comment 2

[:glob ✱]

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

i like the idea, however this change silently assumes that $params won't change during the lifetime of the request, however we don't /guarantee/ that so i'm not comfortable with this (who knows what sort of shenanigans an extension/customisation may do?).

perhaps the cache could be stored within the $params object?

Comment 3

[Simon Green]

Never mind, we'll carry the change locally.

Comment 4

[Simon Green]

Attachment: v2 patch

Now with an added comment.

Comment 5

[:glob ✱]

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

r=glob

Comment 6

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #2)
> i like the idea, however this change silently assumes that $params won't
> change during the lifetime of the request, however we don't /guarantee/ that
> so i'm not comfortable with this

Which is why I wonder if it's a good idea to take this patch on a stable branch (4.4).

Simon: which method did you call in your testing, and which fields did you include in include_fields and/or exclude_fields? I tested your patch with Bug.get(), but I see no significant improvement.

Comment 7

[Simon Green]

> Which is why I wonder if it's a good idea to take this patch on a stable
> branch (4.4).

I'm okay if you only want to take this for Bugzilla 4.5/5.0
 
> Simon: which method did you call in your testing, and which fields did you
> include in include_fields and/or exclude_fields? I tested your patch with
> Bug.get(), but I see no significant improvement.

I've tested it on Product.get, Bug.get and a few brc specific .get calls too. When using Bug.get retrieving 4,000 bugs I saw an improvement from 7.6 seconds to 1.6 seconds.

Unfortunately my workstation is currently unavailable (we had a building wide power outage this weekend), but I will try and get the query later today.

Comment 8

[Simon Green]

(In reply to Frédéric Buclin from comment #6)
> (In reply to Byron Jones ‹:glob› from comment #2)
> > i like the idea, however this change silently assumes that $params won't
> > change during the lifetime of the request, however we don't /guarantee/ that
> > so i'm not comfortable with this
> 
> Which is why I wonder if it's a good idea to take this patch on a stable
> branch (4.4).
> 
> Simon: which method did you call in your testing, and which fields did you
> include in include_fields and/or exclude_fields? I tested your patch with
> Bug.get(), but I see no significant improvement.

This is an example of a call I made on my machine, where the time was cut from 4.41 seconds to 2.45 seconds (using JSONRPC). NYTProf logs have been e-mailed to LpS outside this ticket.

method  => 'Product.get',
params  => {
        Bugzilla_login => 'X',
        Bugzilla_password => 'X',
        names => [ 'Fedora' ],
        include_fields => ['id', 'names', 'components.id', 'components.name', 'components.flag_types' ],
    }

Comment 9

[Frédéric Buclin]

I did something similar:

  my $call = rpc_call('Product.get',
                      {names => ['Big Product'],
                       include_fields => ['name', 'versions', 'components.name',
                                          'components.description', 'classification',
                                          'milestones.name', 'components.flag_types'],
                       exclude_fields => ['versions.sort_key']});

With and without your patch, it takes 31.3 seconds to complete, where Big Product has 1000 versions, 1000 milestones and 1000 components.

NYTProf shows that:

without your patch:

# spent 276ms within Bugzilla::WebService::Util::filter_wants which was called 17004 times, avg 16µs/call:
# 16002 times (259ms+0s) by Bugzilla::WebService::Util::filter at line 33, avg 16µs/call 


with your patch:

# spent 165ms (131+34.2) within Bugzilla::WebService::Util::filter_wants which was called 17004 times, avg 10µs/call:
# 16002 times (123ms+31.9ms) by Bugzilla::WebService::Util::filter at line 33, avg 10µs/call 


So that's a 40% improvement, but my timing is very different from yours as your NYTProf output shows that you called filter_wants() 148'250 times when I only called it 17'000 times. In my testing, the bottleneck is in SOAP::Serializer.

Comment 10

[Simon Green]

(In reply to Frédéric Buclin from comment #9)
> In my testing, the bottleneck is in
> SOAP::Serializer.

Definitely, but that is beyond our direct control.

  -- simon

Comment 11

[Frédéric Buclin]

Just to be clear: an improvement of 40% is still a good thing, so I'm fine taking this patch. :)

Comment 12

[Simon Green]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/WebService/Util.pm
Committed revision 8670.                                                       

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.4/                         
modified Bugzilla/WebService/Util.pm
Committed revision 8583.