537949 - Improve performance when deselecting products/classifications in query.cgi

Status: RESOLVED WORKSFORME

(Bugzilla :: Query/Bug List, enhancement)

Description

[Albert Ting]

Attachment: Suggested 3.4.4 patch

In the advanced query page, selecting a single classification narrows the scope of the available products/components.  When I deselect the classification, Bugzilla resets the list of available products/components.

This works well for the most part.  However, things gets sluggish when you have 1500+ products in multiple classifications. In fact, IE7 aborts with a javascript timeout error.  I believe the main reason is that js/productform.js unnecessarily calls the merge_array function multiple times when one only needs to reset the products/components back to the original list.

The attached 3.4.4 patch addresses this and removes the timeout error I have been seeing in IE7.  Note, the patch may be bitrot a little bit due to customizations I've done on top of this.

Comment 1

[Max Kanat-Alexander]

Cool. Thanks for this patch--I may not be able to get to it very soon, though, because it's really large and it involves modifying some JavaScript that I'm not very familiar with (though I am a little familiar with it). It might be good to make sure that the patch also applies on the tip, so that if and when I do get to it, we don't have to port it forward then.

Comment 2

[Albert Ting]

I agree I should do this based on the tip.  Unfortunately I have limited resource to do that.  

The patch isn't that big.  I changed two functions, and most of it is due to indentation.  The other changes is passing an extra "null" parameter to the modified functions.

Comment 3

[Max Kanat-Alexander]

Hmm, okay. Do you want to post a diff -w to help with review, then?

Comment 4

[Albert Ting]

Attachment: v2

cleaned up bitrot

Comment 5

[Albert Ting]

Attachment: v2 (with diff -w)

Here's the diff -w version

Comment 6

[Max Kanat-Alexander]

Comment on attachment 420281 [details] [diff] [review]
v2 (with diff -w)

>--- js/productform.js.orig	2010-01-05 23:51:11.049259000 -0800
>+    selectProduct(classfield, product, component, version, milestone, null);

  I don't understand why one variable is called classfield while the others are called directly by their field names. Are you just re-using an existing variable?

>@@ -276,6 +289,29 @@
>+function selectAll(items, target, anyval) {

  There's already a function in js/util.js that does most of what this does, besides the anyval bit.

>+++ template/en/default/search/form.html.tmpl	2010-01-05 00:10:10.971035000 -0800
>+[% BLOCK select_list %]

  We might want to call it js_select_list to make it clearer what it's doing, for people who find it at the bottom of the file without reading the top of the file. But that's optional.

>+  var [% sel.jsname %] = new Array();
>+
>+  [% n = 0 %]
>+  [% FOREACH obj = ${sel.varname} %]
>+    [% IF sel.obj == 1 %] [% name = obj.name %] [% ELSE %] [% name = obj %] [% END %]
>+    [% IF sel.name != "product" or obj.components.size %]

  I don't understand why the IF condition there, because there's no ELSE and no other code in this block.

>+      [% sel.jsname %]["[% name FILTER html %]"] = 1;

  You should "FILTER js" the jsname, so that we don't have to add it to filterexceptions (which I generally like to avoid). Also, name needs to be FILTER js, not FILTER html.

  Also, instead of doing things like this, I'd prefer to see you create a unique array in Perl or TT and then just create a unique normal array, instead of a hash in JS.

  I'm also concerned about how much JS this will add to query.cgi, which is already slow on large installations because of the amount of JS it downloads, as I understand it (though I haven't tested that fact).

>+      [% n = n+1 %]

  Also, this variable doesn't seem to be being used.

Comment 7

[Albert Ting]

Attachment: v2 (with diff -w)

(In reply to comment #6)
> (From update of attachment 420281 [details] [diff] [review])
> >--- js/productform.js.orig	2010-01-05 23:51:11.049259000 -0800
> >+    selectProduct(classfield, product, component, version, milestone, null);
> 
>   I don't understand why one variable is called classfield

I'm using the same name that is used in selectClassification().  I don't know why someone approved using "classfield", but felt consistency was more important.  If you like, I can change all classfield to classification.

> >@@ -276,6 +289,29 @@
> >+function selectAll(items, target, anyval) {
> 
>   There's already a function in js/util.js that does most of what this does,
> besides the anyval bit.

The js/util.js version takes as input an array of arrays for "items", plus it doesn't take the anyval bit.  Using an array of arrays slows down array access.  Still, if you like, I can update bz_populateSelectFromArray to accept an anyval param, and update js_select_list to create an array of arrays.

> >+    [% IF sel.obj == 1 %] [% name = obj.name %] [% ELSE %] [% name = obj %] [% END %]
>   I don't understand why the IF condition there, because there's no ELSE 

Updated the indentation so that you see the ELSE statement.

>   Also, instead of doing things like this, I'd prefer to see you create a
> unique array in Perl or TT and then just create a unique normal array, instead
> of a hash in JS.

That's not possible, it needs to be a js array in order to reset the select list in js.  Or perhaps I missing the issue.
 
>   I'm also concerned about how much JS this will add to query.cgi

Another option could be to auto-generate this from the "cpts" array.  While it would cut down the network traffic, it would slow down the javascript execution.  So I think we're better off sending the array.

All other concerns addressed in the latest patch.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> I'm using the same name that is used in selectClassification().  I don't know
> why someone approved using "classfield", but felt consistency was more
> important.  If you like, I can change all classfield to classification.

  Oh, I suppose we'll keep consistency, then. This code will actually eventually all be going away, anyhow, in favor ofcode that uses cascading event handlers instead.

> The js/util.js version takes as input an array of arrays for "items", plus it
> doesn't take the anyval bit.  Using an array of arrays slows down array access.
>  Still, if you like, I can update bz_populateSelectFromArray to accept an
> anyval param, and update js_select_list to create an array of arrays.

  No, no need to add an anyval param. But I think it would be OK to have bz_populateSelectFromArray also take a straight array of strings, if it doesn't slow it down too much.

> > >+    [% IF sel.obj == 1 %] [% name = obj.name %] [% ELSE %] [% name = obj %] [% END %]
> >   I don't understand why the IF condition there, because there's no ELSE 
> 
> Updated the indentation so that you see the ELSE statement.

  That's not the IF I was talking about. I was talking about the one directly above my comment.

> >   Also, instead of doing things like this, I'd prefer to see you create a
> > unique array in Perl or TT and then just create a unique normal array, instead
> > of a hash in JS.
> 
> That's not possible, it needs to be a js array in order to reset the select
> list in js.  Or perhaps I missing the issue.

  Oh, perhaps I was unclear. You're creating named indexes in an array instead of just creating an array of strings. Just create an array of unique strings, and you can figure out the list of unique strings in Perl or TT.

> >   I'm also concerned about how much JS this will add to query.cgi
> 
> Another option could be to auto-generate this from the "cpts" array.  While it
> would cut down the network traffic, it would slow down the javascript
> execution.  So I think we're better off sending the array.

  Okay. I'll need some tests done to see if this makes a significant impact on actual page load time (total time to usability).

Comment 9

[Albert Ting]

(In reply to comment #8)

> if you like, I can update bz_populateSelectFromArray to accept an
> anyval param, and update js_select_list to create an array of arrays.
> 
>   No, no need to add an anyval param. But I think it would be OK to have
> bz_populateSelectFromArray also take a straight array of strings, 
> if it doesn't slow it down too much.

Dumb question, how does a function take both an array of strings as well as an array of array of strings?  Do you want the function to auto-determine the parameter structure?  

>+    [% IF sel.name != "product" or obj.components.size %] 
> > >   I don't understand why the IF condition there, because there's no ELSE 

I only wanted to include products that have components.  Otherwise they are not valid products.  Granted, at the time I felt this was necessary.  Perhaps the products array now only contains products with components. 


> You're creating named indexes in an array instead
> of just creating an array of strings. Just create an array of unique strings,
> and you can figure out the list of unique strings in Perl or TT.

Understood.  Let me check, I'm using this array for other purposes (in my custom environment).

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Dumb question, how does a function take both an array of strings as well as an
> array of array of strings?  Do you want the function to auto-determine the
> parameter structure?  

  Well, either we implement a second function or we check inside the loop what we're getting, for each item. That might be too slow in JS though.

> I only wanted to include products that have components.  Otherwise they are not
> valid products.  Granted, at the time I felt this was necessary.  Perhaps the
> products array now only contains products with components. 

  Oh, well, I wouldn't worry about that. Products that don't have components are a very anomalous case.

Comment 11

[Albert Ting]

(In reply to comment #10)
> (In reply to comment #9)
> > Do you want the function to auto-determine the
> > parameter structure?  
>
>   Well, either we implement a second function or we check inside the loop what
> we're getting, for each item. That might be too slow in JS though.

Then perhaps I should just move the selectAll function to utils.js?  What should the function name be like.
 
> > I only wanted to include products that have components.  Otherwise they are not
> > valid products.  Granted, at the time I felt this was necessary.  Perhaps the
> > products array now only contains products with components. 
> 
>   Oh, well, I wouldn't worry about that. Products that don't have components
> are a very anomalous case.

I think we should worry.  If the array holds an invalid/undefined name, it could mess up the javascript function.

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> Then perhaps I should just move the selectAll function to utils.js?  What
> should the function name be like.

  Just modify bz_populateSelectFromArray. Check the first item in the array--if it's not an array, then the code should behave differently.

  You don't need to include the anyval functionality inside of bz_populateSelectFromArray. All you have to do in that case is push an extra value into your array.

> I think we should worry.  If the array holds an invalid/undefined name, it
> could mess up the javascript function.

  That should be handled in the Perl code; products without components just shouldn't show up.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 420375 [details] [diff] [review]
v2 (with diff -w)

Currently r- based on my various previous comments.

Comment 14

[Frédéric Buclin]

Albert, are you still working on it?

Comment 15

[Albert Ting]

Hi Frederic, unfortunately I haven't had time to update this patch.  Not sure when my next window of Bugzilla work will happen.

Comment 16

[Lalit Khattar]

I did some benchmarking to confirm whether the current js code has performance issues or not.

Added 20 classifications and 100 products under each classification i.e. 20*100 = 2000 products in my    local database.

Below are the results in different browsers:-

1. Google Chrome(version 29): No performance issue. Results are shown within second.
2. Firefox(version 26): No performance issue. Results are shown within second.
3. IE(version 8): No js timeout error, though slow compared to Chrome and Firefox.

Reason for performance improvement in current code from trunk:-

1. Bug 636416 solved this issue. js/productform.js is no longer used in query.cgi, instead js/field.js is used now.

2. Exact reason which I think is there is no sorting and merging step in value-controller code pattern in js/field.js which is not the case in js/productform.js so latter one is relatively slower.

3. Now, every time a source field value is selected/deselected correspondingly target field value is shown/hidden using CSS class i.e. absolutely no merge and sort.

If number of products/components are too high then there might be performance issues so AJAX could be a possible solution as mentioned in bug 65052.

Comment 17

[Frédéric Buclin]

Albert, is this still an issue in Bugzilla 4.4? If not, please close the bug as WFM.

Comment 18

[Albert Ting]

I can't say, as I am still using 4.0!  Haven't had a chance to upgrade.  

I suggest closing this, I will reopen if required.

Comment 19

[:glob ✱]

great work lalit!
marking as WFM as per comment 18.