849102 - mydashboard should lazy-load the 'last change' information

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: MyDashboard, defect)

Description

[:glob ✱]

Attachment: mysql-trace (single bug)

when mydashboard loads a bug list, it selects all fields for each bug in the list from the bugs table, twice.

while the main query for the list looks great, i suspect this is because it's loading bug objects when generating the last-change information.

the last-change information should be lazy-loaded when the triangle toggle is clicked, and should use the object cache to avoid loading a bug multiple times.

Comment 1

[David Lawrence [:dkl]]

I can definitely see where I need to improve things.

Currently for each query I am doing:

1. Using Bugzilla::Search I pass in a query string based on preset standard queries or the user's saved searches. This will grab id, status, delta_ts, and summary only. The results are placed in a list of hashes.

2. I iterate through each bug and call Bugzilla::WebService::Bug->history with the current delta_ts as the start_time. This limits what is returned but ->history will still load the full bug object as part of Bugzilla::Bug->check to make sure the user can access it.

3. And after, I then use Bugzilla::WebService::Bug->comments to load the last comment of the bug. I first find out the comment_id of the last comment using a direct SQL call and then pass in the comment id as a param to ->comments. This limits the amount returned as well but it also calls Bugzilla::Bug->check which reloads the same bug again.

So basically the bugs table is hit 3 times :( and the bugs_activity and longdescs tables 1 time each. Then multiply by the number of bugs returned :( :( 

Couple of ways I can solve this:

1. Optimize the way I load the data and continue to return it as part of the main query call as we do now.
   a. Convert Bugzilla::Bug->check to use cached bugs in the request_cache if they exist. Currently it just calls $class->new($params). This will cut down the amount of bugs tables accesses by 1. Or...
   b. Load the data directly from the database using SQL in my run_bug_query webservice call. Such as the data from bugs_activity and use Bugzilla::Comment->new($comment_id). This would get rid of the calls to Bugzilla::Bug->check as we already know the user can see the bug in question due to coming from Bugzilla::Search.

2. As you suggest, hook into the drop down arrow to load the data from a dedicated webservice call and populate the last changes row at that time. The dedicated call would still need to hit Bugzilla::Bug->check but would only need to do so for the bug requested and not all.

I will work on #2 as it doesn't make sense any more to return all of the change data anyway for all the bugs which the user will probably not even look at.

dkl

Comment 2

[Gervase Markham [:gerv]]

I like being able to see the last change time for each bug, and sort by it, on initial load... Would that feature go away if you implemented this?

Gerv

Comment 3

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #2)
> I like being able to see the last change time for each bug, and sort by it,
> on initial load... Would that feature go away if you implemented this?
> 
> Gerv

glob is referring to the data that you see when you expand the row for a given bug. Currently all of that information is loaded with each refresh regardless if you look at the extra data for each row or not. The updated timestamp for each row will still be there.

dkl

Comment 4

[Gervase Markham [:gerv]]

Ah great, thanks :-)

Gerv

Comment 5

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

Attachment: bug-849102-v1.patch

This patch allows the triangle YUI DataTableRowExpansion to lazy load the last changed information. It doesn't seem to greatly improve performance, on my machine anyway.

Comment 6

[:glob ✱]

Comment on attachment 8432048 [details] [diff] [review]
bug-849102-v1.patch

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

this looks great.  it definitely makes a noticeable difference on large bug lists (eg. 5s down to 1.2s for the table loading rpc call of over 700 bugs).

- there needs to be a 'loading...' displayed while the data is being fetched
- if possible, avoid fetching the same data twice (ie. expand, collapse, expand should only trigger one rpc call)

::: extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl
@@ +67,4 @@
>      {{else}}
>        This is a new [% terms.bug %] and no changes have been made yet.
>      {{/if}}
> +    {{/if}}

the indentation of your changes to this file look wrong (if blocks should be indented)

::: extensions/MyDashboard/web/js/query.js
@@ +167,5 @@
> +            var callback = {
> +                success: function(e) {
> +                    if (e.response) {
> +                      var last_changes = e.response.results[0].last_changes;
> +                      Y.one('#last_changes_' + data.bug_id).setHTML( last_changes_template(last_changes));

nit: incorrect indentation

@@ +196,5 @@
> +                },
> +                callback: callback
> +            });
> +
> +            return last_changes_template({stub: data.bug_id });

nit: missing space after {

Comment 7

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

Attachment: bug-849102-v2.patch

Here is a revised version. I think the javascript is much cleaner (better variable names that are disambiguated between the two similar RPC queries). The variable names I changed are also camelCased as that seemed to be predominate style.
The caching works and I think the indentation is idiomatic (2 spaces in html vs. 4 spaces in javascript).

This results in a larger patch, but I think there wouldn't be time for a cleanup on a little BMO extension such as this.

Comment 8

[:glob ✱]

Comment on attachment 8434408 [details] [diff] [review]
bug-849102-v2.patch

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

r=glob nice!

::: extensions/MyDashboard/template/en/default/pages/mydashboard.html.tmpl
@@ +26,5 @@
>  [% END %]
>  
> +<script id="last-changes-stub" type="text/x-handlebars-template">
> +<div id="last_changes_stub_{{bug_id}}">Loading...</div>
> +</script>

nit: <div> needs to be indented

Comment 9

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

With indented div.

To [e-mail]:webtools/bmo/bugzilla.git
   d7f26d1..1a8d774  master -> master