1243775 - The time taking to fetch a bug's "edit" data leads to an annoying experience when changing components/products

Status: RESOLVED WORKSFORME

(bugzilla.mozilla.org :: User Interface, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=570953
2)  Decide it should be in the "Networking:HTTP" component (yeah, I know,
    humor me).
3)  Click the "Edit" button at the top right.
4)  Click the "Component" combobox.

EXPECTED RESULTS: List of all components drops down.

ACTUAL RESULTS: The dropdown only contains "Networking:FTP".

Presumably the fetch of the full component list is still happening or something; if I immediately close and then reopen the combobox I typically get the list of components I want...

Also, if I take too long between step 3 and step 4 the list pops down correctly.  Of course usually by the time I click the Edit button I know what I want to do, and muscle memory is a thing, so about 90% of the time I get to the combobox and click on it before it's managed to load the component list.

Comment 1

[:glob ✱]

what's happening here is the the ui is switched into edit mode so you can start making changes, while in the background a xhr request is kicked off to grab the data needed to populate the products, components, etc.

we could not switch immediately to edit mode, instead wait for the xhr response, but i don't like this idea.

perhaps we should disable the selects that we know are waiting for data, enabling them for editing once the data arrives.

Comment 2

[Boris Zbarsky [:bzbarsky]]

That would have its own annoyance: I would click the "edit" button, then click the combobox and nothing would happen...

How about kicking off the XHR when the mouseover event fires on the edit button or something?  On slow enough connections it would all be just bad no matter what, of course.  :(

Comment 3

[:glob ✱]

(In reply to Boris Zbarsky [:bz] from comment #2)
> That would have its own annoyance: I would click the "edit" button, then
> click the combobox and nothing would happen...

yeah :|

> How about kicking off the XHR when the mouseover event fires on the edit
> button or something?  On slow enough connections it would all be just bad no
> matter what, of course.  :(

.. and it would also create unnecessary requests against bmo just because someone moved their mouse over the button, which i'm keen to avoid.


i suspect the real fix for this is to speed up the xhr request:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BugModal/lib/WebService.pm#L66

unfortunately there's an overhead in bugzilla's webservice layer that can only be eliminated with essentially a rewrite, however if we're careful with caching and per-user permissions we could make some gains by ensuring we lean on memcached as much as possible (eg. Bugzilla->active_custom_fields seems ripe for memcaching).

Comment 4

[:glob ✱]

> Presumably the fetch of the full component list is still happening or
> something; if I immediately close and then reopen the combobox I typically
> get the list of components I want...

fwiw i've never had to close and reopen the combo to get the populated list.  ie. if i open it and there's only one item, waiting (generally less than a second) results in a fully populated combo with any further actions.  that you have to close then reopen to see an updated list is a surprise.

Comment 5

[:glob ✱]

(with all the combo box playing i was bound to accidentally update the component eventually; moving back)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> that you have to close then reopen to see an updated list is a surprise.

Are you using a nightly (hence e10s)?  I expect that non-e10s comboboxes update properly and e10s ones do not... and I expect correctly, looks like.  Filed bug 1244007.

But note that comboboxes that are dropped down also don't update on DOM mutation in Chrome or Safari on Mac, at least.

Comment 7

[:glob ✱]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Are you using a nightly (hence e10s)?  I expect that non-e10s comboboxes
> update properly and e10s ones do not... and I expect correctly, looks like. 

ah, devedition without e10s here.

> But note that comboboxes that are dropped down also don't update on DOM
> mutation in Chrome or Safari on Mac, at least.

noted.  my comment wasn't meant to indicate this issue shouldn't be fixed, sorry if it came across that way.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> my comment wasn't meant to indicate this issue shouldn't be fixed

I didn't think it was, fwiw.  I figure we're talking about priority here, not whether it should be fixed in some Platonic sense.  ;)

Comment 9

[:glob ✱]

was thinking about this last night.

we could create a cache of the values in local storage (per-user and per-product).

when you hit edit we initially populate the selects from the local-storage cache, then kick off the xhr request.
the response handler would update local-storage as well as the current values in the selects.

we'd need to ensure that if the user selects a value from local storage that is no longer valid after the xhr update, the select is tagged as requiring updating and you cannot submit the bug (a client-side error is nicer than our server-side one).

Comment 10

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

(In reply to Byron Jones ‹:glob› from comment #3)
> i suspect the real fix for this is to speed up the xhr request:
> https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/
> BugModal/lib/WebService.pm#L66
> 

There main slow points are the active_custom_fields hook in tracking flags (for several reasons)
and the deserialization of the bz schema. 

The script I'm using to profile and my analysis will follow.

Comment 11

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

Attachment: profile-bug-modal-webservice.pl

Comment 12

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

Attachment: 1243775_1.patch

Here's what we know: spent 32.7s (293ms+32.4) within Bugzilla::Extension::BugModal::WebService::edit which was called 100 times, avg 327ms/call:

Looking at the flame chart on http://people.mozilla.org/~dhardison/1243775/nytprof.before/ we can begin to zero in on the hot parts.

In the left quadrant you'll notice active_custom_fields, which :glob has suggested caching. We can do that. But wait a second,
why are we calling that from DB_COLUMNS?

http://people.mozilla.org/~dhardison/1243775/nytprof.before/Bugzilla-Bug-pm-73-line.html#84
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/Bugzilla/Bug.pm#L86-L88

Custom fields are DB columns, except for multi-selects and extensions (which I think is only tracking flags at the moment?)

So our first optimization can be to never bother including FIELD_TYPE_EXTENSION when we're never going to need it.

Here is the difference for the first patch:
# spent 17.5s (284ms+17.3) within Bugzilla::active_custom_fields which was called 200 times, avg 87.7ms/call:
http://people.mozilla.org/~dhardison/1243775/nytprof.before/Bugzilla-pm-2-line.html#707

# spent 5.69s (160ms+5.53) within Bugzilla::active_custom_fields which was called 200 times, avg 28.4ms/call:
http://people.mozilla.org/~dhardison/1243775/nytprof.patch1/Bugzilla-pm-2-line.html#707

Note this will speed up all uses of Bugzilla::Bug->new()

Comment 13

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

Attachment: 1243775_2.patch

After patch1 is applied, we can look at another interestingly slow piece:

http://people.mozilla.org/~dhardison/1243775/nytprof.patch1/Bugzilla-Bug-pm-73-line.html#4639
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/Bugzilla/Bug.pm#L4639-L4659

create_cf_accessors creates methods on the Bugzilla::Bug class. It's a little smart -- it caches if it did this.
Unfortunately it does this in the request cache. It also keys on the class -- useful if Bugzilla::Bug was being subclassed.
I'm not sure if that is important, but we can could instead cache this in a file-scoped lexical which means we only have to do this once per process.

profiling this second patch (with patch1 also applied) looks like this:
http://people.mozilla.org/~dhardison/1243775/nytprof.patch2

Now we can't even see where we create the custom accessors!

Comment 14

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

Of course, if you look you'll see that Bugzilla::Bug->check() is now 30% of our time.
Following the flame chart up, you'll see the biggest contribution is... active_custom_fields!

But is it really the problem? Looking above it we see most of the time is being taken by...
http://people.mozilla.org/~dhardison/1243775/nytprof.patch2/Bugzilla-DB-pm-49-line.html#1411
or even more specifically http://people.mozilla.org/~dhardison/1243775/nytprof.patch2/Bugzilla-DB-Schema-pm-78-line.html#3038

The real fix for that one is bug 1032080 (my preference is Sereal) but what if we cache it in the process too?
I'm not confident in this change, but here are the results:

http://people.mozilla.org/~dhardison/1243775/nytprof.patch3/Bugzilla-DB-pm-49-line.html#1411

so we can go from http://people.mozilla.org/~dhardison/1243775/nytprof.before
# spent 32.7s (293ms+32.4) within Bugzilla::Extension::BugModal::WebService::edit which was called 100 times, avg 327ms/call

to http://people.mozilla.org/~dhardison/1243775/nytprof.patch3
# spent 12.5s (219ms+12.3) within Bugzilla::Extension::BugModal::WebService::edit which was called 100 times, avg 125ms/call

now it would be curious to see how much caching active_custom_fields() would be...

Comment 15

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

If we use Cache::Memcached::Fast and drop the (useless) detainting, we can get it down to 83.1ms

# spent 8.31s (222ms+8.08) within Bugzilla::Extension::BugModal::WebService::edit which was called 100 times, avg 83.1ms/call

http://people.mozilla.org/~dhardison/1243775/nytprof.patch5/WebService-pm-73-line.html#66

That will have to wait until after the upstream merge.

Comment 16

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

Attachment: 1243775_3.patch

This patch makes it so edit mode is not turned on until after the XHR request. This means a 900ms to 1s delay for some bugs (like bug 570953).

I did not pursue caching in this bug as the two hardest things in CS are cache invalidation and naming things. 

If I do client side caching now it doesn't improve the experience the first time someone goes to edit a bug, unless I also kick off the XHR on page load. Doing that will result in more server load.

What needs to be done is a caching strategy for the result of bug_modal's private edit API. Combined with the other performance improvements I've defined, we can make bug_modal's edit API *really* fast. 

Since we would have already solved (ha ha) the cache invalidation problem, we should be able to use key/etag to let the browser cache the result of the REST API.

Comment 17

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #16)
> Created attachment 8756944 [details] [diff] [review]
> 1243775_3.patch
> 
> This patch makes it so edit mode is not turned on until after the XHR
> request. This means a 900ms to 1s delay for some bugs (like bug 570953).

please consider BMO is a global resource and not all of us have low latency connections to the US.

Comment 18

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

I think this patch provides a slightly better behavior (waiting for the XHR to be done) but we must also account for latency.

One of the best ways of not having a degrading experience on a high-latency connection would be to include initial values in the first show_bug request. Doing that now is not palatable because the information required can take >= 900ms to generate and we want show_bug to be fast. 

I should back up and explain my context (I did try several experiments with this)

1) fix the widget
 Using selectize() would be nice. Start typing and when the data is loaded, whatever you've typed can be matched to what the values we get from the webservice are. The choice is discounted because styling selectize to look right on all BMO pages (not just showbug) is hard.

2) cache at page load
this would introduce load on Bugzilla (because edit() is resource intensive)

3) cache at first edit request
This does not fix the reported problem. I'd like the edit button to always be fast, not just if you happen to have the right data in the local storage.

4) cache all products and all components. Possibly include them in the HTML
This could be reasonably fast (User->get_enterable_products() is slow but you can write much better SQL to get all that info,
and memcached the structure. Needs to be cached in a way that user group changes or product or component name changes invalidate it, which means some additional support to Bugzilla::Memcached.

Going down this road lead me to thinking of using memcached for the edit API. My napkin math says caching this per-user for every active user would be about 370MB

If the edit API is really fast, we could just as well include it in the show_bug.cgi output (for authenticated users, as non-auth'd can't edit) and use that, updating it with the result of XHR callback.

Comment 19

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

The experience of clicking the "edit" bug is critical for this feature.

Comment 20

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #18)
> 3) cache at first edit request
> This does not fix the reported problem. I'd like the edit button to always
> be fast, not just if you happen to have the right data in the local storage.

this is in opposition to your fix in the attached patch.


given the local storage cache is per-product, i think it's a good enough solution balanced between reducing both server load and edit time.  the cache invalidation problem is a non-issue: always replace the local cache with the result from the server.

for light bugzilla users, yes, the initial edit will be slower.  its performance would at worst match the change from attachment 8756944 [details] [diff] [review].  if this is a concern then you could initiate a XHR result on page load if the local cache for the current product doesn't exist.

heavier users are much more likely to have edited bugs in the same product before, and would gain an instantaneous edit switch.

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 8756944 [details] [diff] [review]
1243775_3.patch

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

r=dkl

Comment 22

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

Attachment: 1243775_4.patch

I reworked this code a bit to use caching in a sort of dumb way. Thoughts?

Comment 23

[David Lawrence [:dkl]]

Comment on attachment 8771227 [details] [diff] [review]
1243775_4.patch

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

$ patch -p1 < 1243775-8771227.patch 
patching file extensions/BugModal/lib/WebService.pm
patching file extensions/BugModal/template/en/default/bug_modal/header.html.tmpl
patching file extensions/BugModal/web/bug_modal.js
Hunk #2 FAILED at 158.
Hunk #4 succeeded at 564 (offset -2 lines).
Hunk #5 succeeded at 1178 (offset 33 lines).
Hunk #6 succeeded at 1367 (offset 33 lines).
Hunk #7 succeeded at 1395 (offset 33 lines).
1 out of 7 hunks FAILED -- saving rejects to file extensions/BugModal/web/bug_modal.js.rej

::: extensions/BugModal/web/bug_modal.js
@@ +583,5 @@
> +                        $('#cancel-btn').prop('disabled', false);
> +                        $('#top-save-btn').show();
> +                        $('#cancel-btn').show();
> +                        $('#commit-btn').show();
> +                    }

Same here, no need for is_cached at all IMO. You still need to hide the mode button in either case.

function(data) {
   populate_selects(data.options, true);
   populate_keywords(data.keywords);
                     
   $('#mode-btn').hide();
   $('#cancel-btn').prop('disabled', false);
   $('#top-save-btn').show();
   $('#cancel-btn').show();
   $('#commit-btn').show();
},

@@ +1154,5 @@
>                  },
> +                function(data, is_cached) {
> +                    if (!is_cached) {
> +                        $('#product-throbber').hide();
> +                    }

I don't see the need for is_cached here at all. If the value return was cached, the throbbers are not removed that are enabled earlier. I would just do:

function(data) {
    populate_selects(data.options, false);
    populate_groups(data.groups_html);
    $('#product-throbber').hide();
    $('#component, #version, #target_milestone').attr('disabled', false);
},

@@ +1337,5 @@
> +
> +    if (request.cache_key) {
> +        var old_data = localStorage.getItem('bugzilla_ajax:' + request.cache_key);
> +        if (old_data) {
> +            done_fn(JSON.parse(old_data), true);

Need to return here otherwise you still perform the fetch each time even for products we already fetched.

if (old_data)
    return done_fn(old_data);

Comment 24

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

Attachment: 1243775_1.patch

Totally new approach. 

1) add memcache caching to the new_product API
2) add ETag support to that API too
3) our current version of jquery can't use etags right now, but 1.4 can...
4) meanwhile, we can remember the last etag we got and use that value prior to making the request and pretend the request worked. This means done_fn() in bugzilla_ajax request is called more than once.

this is feedback now as it needs one more piece of logic: if
the user changes any values between the first (synthesized) done callback,
the second (real) call shouldn't override those.

Comment 25

[David Lawrence [:dkl]]

(In reply to Dylan Hardison [:dylan] from comment #24)
> Created attachment 8803996 [details] [diff] [review]
> 1243775_1.patch
> 
> Totally new approach. 
> 
> 1) add memcache caching to the new_product API
> 2) add ETag support to that API too
> 3) our current version of jquery can't use etags right now, but 1.4 can...
> 4) meanwhile, we can remember the last etag we got and use that value prior
> to making the request and pretend the request worked. This means done_fn()
> in bugzilla_ajax request is called more than once.
> 
> this is feedback now as it needs one more piece of logic: if
> the user changes any values between the first (synthesized) done callback,
> the second (real) call shouldn't override those.

This sounds like a sane plan that I would like to see how well it works in practice. Use of etags and memcache would be of great help obviously. Should we update jquery to the latest version above 1.4 or are there issues with jquery 3.x that may affect us?

dkl

Comment 26

[Mark Côté [:mcote]]

Not sure if dkl's question in comment 25 is blocking feedback here, but might as well get it answered. :)

Comment 27

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

(In reply to David Lawrence [:dkl] from comment #25)
> (In reply to Dylan Hardison [:dylan] from comment #24)
> > Created attachment 8803996 [details] [diff] [review]
> > 1243775_1.patch
> > 
> > Totally new approach. 
> > 
> > 1) add memcache caching to the new_product API
> > 2) add ETag support to that API too
> > 3) our current version of jquery can't use etags right now, but 1.4 can...
> > 4) meanwhile, we can remember the last etag we got and use that value prior
> > to making the request and pretend the request worked. This means done_fn()
> > in bugzilla_ajax request is called more than once.
> > 
> > this is feedback now as it needs one more piece of logic: if
> > the user changes any values between the first (synthesized) done callback,
> > the second (real) call shouldn't override those.
> 
> This sounds like a sane plan that I would like to see how well it works in
> practice. Use of etags and memcache would be of great help obviously. Should
> we update jquery to the latest version above 1.4 or are there issues with
> jquery 3.x that may affect us?
> 
> dkl

We upgraded to jquery 3.x so I'll retest this patch.

Comment 28

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

I'm going to deploy this to dev for further manual QA before review. I'll write up a simple test plan too.

Comment 29

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Thanks. You can also set the magical latency simulator up in a couple of web debuggers.

Comment 30

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Is this still on dev?

Comment 31

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

Comment on attachment 8803996 [details] [diff] [review]
1243775_1.patch

I'm going to seek yet another approach on this, as many other constraints have changed.

Comment 32

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Are we using the new API for querying on product/component here?

Comment 33

[Kohei Yoshino]

No, the product/component list on the New Bug page is user-specific. I’ll think about how to make this better.

Comment 34

[Kohei Yoshino]

Thanks to Dylan’s performance work, the API has become a lot faster since when this bug was filed 3 years ago. There’s still room for enhancements in the UX overall, but I think this particular bug can be closed now. In the future:

• Make Bugzilla (almost) single-page progressive web app
• Cache the product/component list and other data locally and update them when notified via the new GraphQL API
• Allow to edit each field or section instead of all the fields

Comment 35

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I agree that this is much better now; the delays are small enough that I haven't run into problems in practice recently.