DeuxHuitHuit / quicksearch

A jQuery plugin for searching through DOM Elements quickly
https://www.npmjs.org/package/jquery.quicksearch
Other
135 stars 35 forks source link

Performance of caching #5

Closed cntoplolicon closed 10 years ago

cntoplolicon commented 10 years ago

quicksearch seems to have some redundant element selection when caching:

this.cache = function (doRedraw) {
            doRedraw = (typeof doRedraw === "undefined") ? true : doRedraw;
            jq_results = options.noResults ? $(target).not(options.noResults) : $(target);
            var t = (typeof options.selector === "string") ? jq_results.find(options.selector) : $(target).not(options.noResults);

In my project, both $(target) and $(target).not(options.noResults) are quite expensive. Wouldn't it be nice if the selection is done only once?

nitriques commented 10 years ago

I can't to that without risking to break things, like dynamic data source (like ajax calls), so it has to stay.

And by expensive, what do you mean ? Time to complete ? Can you provide benchmarks on different browsers ?

Thanks!

cntoplolicon commented 10 years ago

I'm using quicksearch to filter a dynamic list having approximately 3000 items, so I have to invoke cache() whenever the underlying list gets updated, which completes instantly on all modern browsers but takes a few seconds on IE10. I'm using the default options: $('input#search').quicksearch('ul#qs li')

The problem is that $(target) is calculated twice during the declaration of jq_results and var t, which seems unnecessary. $(target) might take a few seconds because it can return thousands of elements.

nitriques commented 10 years ago

calling $(target) twice should not be a problem... target should already be saved as a jQuery object... which version of jquery are you using ??? Do you have migrate or not ?

Do you have a working fix ? I am willing to try it....

cntoplolicon commented 10 years ago

Sorry... IE10 was not the whole story. It's a legacy project and the document mode is 7. Modern browsers like Chrome can also block if options.noResults is set.

I am using jQuery 1.10.2 without migration. I think target is a raw string rather than a jQuery object.

In standard-compliant browsers like Chrome, although document.querySelectorAll is available, not(option.noResults) is not using it, which has performance penalty. In legacy browsers like IE7, document.querySelectorAll is not available at all. In this case jQuery resorts to Sizzle to select elements, which can be quite slow even if options.noResults is not set. Anyway, calling $(target) or $(target).not(option.noResults) twice doesn't seem to be a good idea.

Another problem is that in cache():

jq_results = options.noResults ? $(target).not(options.noResults) : $(target);

is essentially unnessary since options.noResults always evaluates to true:

options.noResults = !options.noResults ? $() : $(options.noResults);

Here is a working fix.

I put things together here. The fixed function is renamed to recache. You can try it on Chrome. You'll probably need to reduce the size of the list if you would like to try it on IE7.

nitriques commented 10 years ago

Great this is nice. Could you send a PR please ? This will allow me to easily do a diff.... Thanks!

nitriques commented 10 years ago

6 merged and shipped! http://plugins.jquery.com/jquery.quicksearch/ Thanks again!