173571 - Turn "all selected" into "none selected" for efficiency

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Gervase Markham [:gerv]]

Selecting all the entries in an listbox on the query page is equivalent to
selecting none of them, but about twice as slow. buglist.cgi should spot when
all entries have been selected, and turn it into the "none" case.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This does the job for status and resolution, which I suspect are the common
cases.

Gerv

Comment 2

[David Lawrence [:dkl]]

You could have query.cgi pass a num_status and num_resolution to buglist.cgi
giving the max number of entries to get rid of the need to hardcode 7 and 9 into
buglist.cgi. I know some, me for example, who will alter the number of entries
displayed in the list boxes in the templates and would need to change the numbers.
I also know that this will be solved when customizable resolutions is
implemented but passing the number of elements should solve it in the meantime.

Comment 3

[Gervase Markham [:gerv]]

Hmm. That's rather a hacky solution, really (like mine, in fact :-). Bugzilla
should be able to work this out for itself without needing that. 

Myk, Dave - any ideas? Would unconditionally doing GetVersionTable() be a bigger
slowdown than this is a speed up? Is calling GetVersionTable() expensive? Can we
do a cheap query to find these numbers?

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

GetVersionTable is currently fairly cheap. It will need to vanish, eventually,
though. In this case, though, custres will break this hack.

We would, of course, like the db to do this for us :)

Comment 5

[Gervase Markham [:gerv]]

So is doing GetVersionTable unconditionally worth it? 

Having been the happy recipient of the mysqld-watcher "I killed this query
because it took too long" emails for the past 24 hours, a significant proportion
(half?) have all the statuses selected. So I think it is worth it.

New patch coming up.

Gerv

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Try this.

Gerv

Comment 7

[Gervase Markham [:gerv]]

An analysis of 17 killed queries showed this problem in 5 of them.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

Comment on attachment 102514 [details] [diff] [review]
Patch v.2

Doing the tests with |scalar| is nicer.

You need to test that the contents are the same, since we never validate that
the status/resolutions selected are actually valid.

Alos, the GetVersionTable call is fragile - I know that report.cgi does this
unconditionally as well, but maybe Bugzilla::Search should, instead?

Comment 9

[Gervase Markham [:gerv]]

bbaetz:

> Doing the tests with |scalar| is nicer.

OK.

> You need to test that the contents are the same, since we never validate that
> the status/resolutions selected are actually valid.

If people are hacking the search form, they deserve everything they get,
including incorrect results. 

> Alos, the GetVersionTable call is fragile - I know that report.cgi does this
> unconditionally as well, but maybe Bugzilla::Search should, instead?

"Fragile"? Maybe Bugzilla::Search should be calling GetVersionTable(), but this
works, is a minimal change, and I want to get it in before b.m.o. upgrades. We
can open a new bug to think about exactly where we should be calling it if you like.

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

No, I mean that relying on the caller having loaded the versiontable is fragile.

If Bugzilla::Search needs to ensure that the version table has been loaded, then
it should do so itsself.

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Nits picked; Search.pm now calls GetVersionTable() for itself. Looking for
r=bbaetz; are you going to make me file a review request? :-)

gerv

Comment 12

[Bradley Baetz (:bbaetz)]

No, it means you find somsone who doesn't have a thesis due in two weeks :)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Wouldn't this work better on the client side as a JS script that unselected the
items on form submission?

Comment 14

[Gervase Markham [:gerv]]

myk: what advantages would that have? It would mean it wouldn't work for people
with no JS, and would increase the size of the page (and the query page is
already pretty big.) Also, can you get the number of items in a select list from
JS? (You probably can; I'm just asking.)

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Yeah, I don't think we want this done via js. Speaking of JS, though, what does
the ALL for quicksearch do?

Anyway, I just thought of the reason why mysql can't do this - because it
accepts 'invalid' enum values, which are hidden by what we currently do, but
wouldn't be with the removal of the clause. I don't think we care about that
distinction, do we?

Comment 16

[Gervase Markham [:gerv]]

No - if there are invalid enum values, too bad. :-) If people add new states in
a way that makes them show up in GetVersionTable, then this code will work. And
that's perfectly good enough. :-)

Gerv

Comment 17

[Myk Melez [:myk] [@mykmelez]]

The main advantage to using JS is that we don't have to depend on the version
cache, so when it goes away we don't have to change any code.  Minor advantages
include cleaner search URLs and probably a small performance improvement for
buglist.cgi.

The fact that some people don't have or use JS is a non-issue.  Almost everybody
does have and use it, and the search form will still work for those who don't.

Comment 18

[Gervase Markham [:gerv]]

> The main advantage to using JS is that we don't have to depend on the version
> cache, so when it goes away we don't have to change any code.

If there's a backwards-compatible interface, this won't be a problem - and if
there isn't, we have a load of code to change, and a couple more lines won't
make a difference.

> Minor advantages include cleaner search URLs and probably a small performance 
> improvement for buglist.cgi.

...which would be cancelled out by even one person doing a stupid search that
the JS didn't prevent because they had it switched off. Now I've changed the
search defaults, this is a very common feature of the remaining mysqld-watcher
mails.

Gerv

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 103907 [details] [diff] [review]
Patch v.3

r=justdave

Comment 20

[Gervase Markham [:gerv]]

Fixed, after conversion to CGI.pm syntax.

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.25; previous revision: 1.24
done


Gerv

Comment 21

[Dave Miller [:justdave]]

fixing target