Make /bzapi/configuration faster

VERIFIED FIXED

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: dylan, Assigned: dylan)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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?
Attachment #8864619 - Flags: review?(glob)
(Assignee)

Updated

2 years ago
Flags: needinfo?(klibby)
Probably ok, but we should keep an eye on memory usage. Definitely worth it, though, if it's that much of a speed up.
Flags: needinfo?(klibby)

Comment 2

2 years ago
(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

2 years ago
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 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.
(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

2 years ago
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

2 years ago
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.)
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
(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...

Updated

2 years ago
Attachment #8864619 - Flags: feedback+

Comment 12

2 years ago
(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 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()
Attachment #8864619 - Flags: review?(glob) → review+
(Assignee)

Comment 14

2 years ago
To git@github.com:mozilla-bteam/bmo.git
   db7ebb9..496cdc3  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(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).
(Assignee)

Comment 16

2 years ago
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

2 years ago
Blocks: 1372539
You need to log in before you can comment on or make changes to this bug.