1362151 - Make /bzapi/configuration faster

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[Dylan Hardison [:dylan] (he/him)]

Attachment: bzapi_config_cache.patch

This patch takes the mentioned API from 7s to 450ms on my development machine. 
This will require increasing the max size of a memcached item to about 2M.


fubar: any objections to setting OPTIONS="-I 2M" for memcached?

Comment 1

[Kendall Libby [:fubar] (he/him)]

Probably ok, but we should keep an eye on memory usage. Definitely worth it, though, if it's that much of a speed up.

Comment 2

[:Atoll]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #0)
> This patch takes the mentioned API from 7s to 450ms on my development
> machine. 
> This will require increasing the max size of a memcached item to about 2M.

In production, how many instances of this key can we expect to be cached in the shared memcache instance? If it's "one per worker per server", is that 1 or 1024 or 1048576?

Comment 3

[:Atoll]

Comment on attachment 8864619 [details] [diff] [review]
bzapi_config_cache.patch

Review of attachment 8864619 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BzAPI/lib/Resources/Bugzilla.pm
@@ +131,4 @@
>      if ($json) {
> +        my $result = $self->json->decode($json);
> +        if ($can_cache) {
> +            Bugzilla->memcached->set_config({key => $cache_key, data => $result});

On the surface, this doesn't seem to handle the case where the blob is larger than the max_size configured in Memcached.pm. How are we alerted to increase the max_size in that scenario?

Comment 4

[:glob ✱]

Comment on attachment 8864619 [details] [diff] [review]
bzapi_config_cache.patch

Review of attachment 8864619 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Memcached.pm
@@ +36,4 @@
>          $self->{memcached} = Cache::Memcached::Fast->new({
>              servers   => [ split(/[, ]+/, $servers) ],
>              namespace => $self->{namespace},
> +            max_size => 1024 * 1024 * 2,

did you test how well this compresses?
we may be able to avoid needing to double the limit by storing it compressed in memcached.

Comment 5

[:glob ✱]

(In reply to Richard Soderberg [:atoll] from comment #2)
> (In reply to Dylan Hardison [:dylan] (he/him) from comment #0)
> > This patch takes the mentioned API from 7s to 450ms on my development
> > machine. 
> > This will require increasing the max size of a memcached item to about 2M.
> 
> In production, how many instances of this key can we expect to be cached in
> the shared memcache instance? If it's "one per worker per server", is that 1
> or 1024 or 1048576?

there should only be one active copy in the cache.
there may be multiple copies depending on the interaction between bmo config changes and requests.

set_config() stores values in memcached with a prefix which is incremented when any configuration value is changed.  older items in the cache are no longer accessed and left to expire.

Comment 6

[:Atoll]

How often does the BZ memcache instance emit "object too large" warnings? If it's never, then you could set this limit to 16MB and see a total memory usage of increase of a single 2MB object, as nothing else that large is being stored.

Comment 7

[:Atoll]

Also, consider whether a Cache-Control (public variant) is appropriate to inject here. (Might be something to do in a later pass about CC specifically, but since this is neither private/no-cache nor public/immutable.)

Comment 8

[Dylan Hardison [:dylan] (he/him)]

(In reply to Richard Soderberg [:atoll] from comment #3)
> Comment on attachment 8864619 [details] [diff] [review]
> bzapi_config_cache.patch
> 
> Review of attachment 8864619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: extensions/BzAPI/lib/Resources/Bugzilla.pm
> @@ +131,4 @@
> >      if ($json) {
> > +        my $result = $self->json->decode($json);
> > +        if ($can_cache) {
> > +            Bugzilla->memcached->set_config({key => $cache_key, data => $result});
> 
> On the surface, this doesn't seem to handle the case where the blob is
> larger than the max_size configured in Memcached.pm. How are we alerted to
> increase the max_size in that scenario?

Caching simply does not happen when the value is too large.

Comment 9

[Dylan Hardison [:dylan] (he/him)]

(In reply to Byron Jones ‹:glob› from comment #4)
> Comment on attachment 8864619 [details] [diff] [review]
> bzapi_config_cache.patch
> 
> Review of attachment 8864619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Bugzilla/Memcached.pm
> @@ +36,4 @@
> >          $self->{memcached} = Cache::Memcached::Fast->new({
> >              servers   => [ split(/[, ]+/, $servers) ],
> >              namespace => $self->{namespace},
> > +            max_size => 1024 * 1024 * 2,
> 
> did you test how well this compresses?
> we may be able to avoid needing to double the limit by storing it compressed
> in memcached.

Compression is automatic and triggered by an input larger than 10_000 bytes.
Storable (the default for Cache::Memcached::Fast) has more overhead than Sereal (https://blog.booking.com/static/sereal-a_binary_serialization_format/output_size.png)
but I didn't want to add that dependency right now.

Comment 10

[Dylan Hardison [:dylan] (he/him)]

(In reply to Richard Soderberg [:atoll] from comment #7)
> Also, consider whether a Cache-Control (public variant) is appropriate to
> inject here. (Might be something to do in a later pass about CC
> specifically, but since this is neither private/no-cache nor
> public/immutable.)

I'm not sure how to retrofit this into the bzapi (or the native rest api) at the moment.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

(In reply to Richard Soderberg [:atoll] from comment #6)
> How often does the BZ memcache instance emit "object too large" warnings? If
> it's never, then you could set this limit to 16MB and see a total memory
> usage of increase of a single 2MB object, as nothing else that large is
> being stored.

Explain? I'd like a larger limit in general for storing pre-rendered comments for non-logged-in users...

Comment 12

[:Atoll]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #11)
> (In reply to Richard Soderberg [:atoll] from comment #6)
> > How often does the BZ memcache instance emit "object too large" warnings? If
> > it's never, then you could set this limit to 16MB and see a total memory
> > usage of increase of a single 2MB object, as nothing else that large is
> > being stored.
> 
> Explain? I'd like a larger limit in general for storing pre-rendered
> comments for non-logged-in users...

I think I'd like to make it a separate bug, to avoid derailing this one. The 2MB increase can be proven operationally and is easy to reverse if needed, f+ from me.

Comment 13

[:glob ✱]

Comment on attachment 8864619 [details] [diff] [review]
bzapi_config_cache.patch

Review of attachment 8864619 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: extensions/BzAPI/lib/Resources/Bugzilla.pm
@@ +131,4 @@
>      if ($json) {
> +        my $result = $self->json->decode($json);
> +        if ($can_cache) {
> +            Bugzilla->memcached->set_config({key => $cache_key, data => $result});

to follow on from atoll's point about alerting; it's probably a good idea to log when any memcached set fails - ie. in Bugzilla::Memcached::_set()

Comment 14

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   db7ebb9..496cdc3  master -> master

Comment 15

[Kendall Libby [:fubar] (he/him)]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #0)
> Created attachment 8864619 [details] [diff] [review]
> bzapi_config_cache.patch
> 
> This patch takes the mentioned API from 7s to 450ms on my development
> machine. 
> This will require increasing the max size of a memcached item to about 2M.
> 
> 
> fubar: any objections to setting OPTIONS="-I 2M" for memcached?

This is in place (aac582abaa). I also had to modify the memcached module to allow managing options, though (745576b25e).

Comment 16

[Dylan Hardison [:dylan] (he/him)]

We moved the decimal point: 

May 13 04:14:51 web1.bugs.scl3.mozilla.com apache[18488]: [Sat May 13 04:14:51 2017] [warn] [request_time] /bzapi/configuration took 6.31371092796326 seconds to execute
May 16 13:41:37 web1.bugs.scl3.mozilla.com apache[14748]: [Tue May 16 13:41:37 2017] [warn] [request_time] /bzapi/configuration took 0.703311920166016 seconds to execute