Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Conversation

nsaleh
Copy link
Contributor

@nsaleh nsaleh commented Apr 19, 2011

As I experienced huge performanceproblems with data-filter on long lists, I optimized it by:

@toddparker
Copy link
Contributor

Thanks, we'll take a look!

@toddparker
Copy link
Contributor

Thanks again, we're a little behind on reviews but we'll take a look as soon as we can.

@eddiemonge
Copy link
Contributor

Can you format your changes according to http://docs.jquery.com/JQuery_Core_Style_Guidelines please?

@nsaleh
Copy link
Contributor Author

nsaleh commented Apr 29, 2011

done, hope i didn't miss anything.
edif: ahhh okay i see now, my xcode doesn't show me the truth ... i'll fix it

@ianeiko
Copy link

ianeiko commented Apr 30, 2011

thanks nsaleh, its a very significant performance boost!

the official version just hangs when testing this on iOS 4 : http://jquerymobile.com/demos/1.0a4.1/#docs/api/../forms/../api/../../docs/lists/lists-performance.html

@hakanson
Copy link
Contributor

I had similar data-filtertext thoughts in pull request 1221 (#1221), but being new to github, my pull request was overload with changes and I only got around to resubmitting one of them.

Also, I got faster show/hide performance based on http://www.learningjquery.com/2010/05/now-you-see-me-showhide-performance.

@nsaleh
Copy link
Contributor Author

nsaleh commented May 2, 2011

@hakanson great link! I'll include the class-based hiding soon.

- added class-based hiding
@nsaleh
Copy link
Contributor Author

nsaleh commented May 9, 2011

I added class-based hiding and finally replaced all spaces with tabs

@@ -772,15 +772,16 @@
return false;
}

$activeClickedLink = $this.closest( ".ui-btn" );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to take the performance part of this patch, but perhaps not this part. I think this is no longer needed.

@jblas jblas merged commit 4ffb0a5 into jquery-archive:master May 9, 2011
@jblas
Copy link
Contributor

jblas commented May 9, 2011

Ok, I landed the patch minus the jquery.mobile.navigation.js changes:

1387fd4

I'd still like to know if there's a specific reason the patch used toggleClass() with true/false state specified versus just calling the appropriate addClass/removeClass. The latter just seems easier to read/understand.

@jblas
Copy link
Contributor

jblas commented May 9, 2011

One other possible issue would be the initial check for jqmData("lastVal") ... it may return undefined in which case the initial comparison may be against a string of "undefined".

  • Kin

@toddparker
Copy link
Contributor

Great, thanks Kin!

@toddparker
Copy link
Contributor

Seems to take less than a second to process a keystroke on the 500 items list performance test on an iPhone 4. Pretty snappy.

@eddiemonge
Copy link
Contributor

The toggleClass part looks like it is supposed to be based on the childItems part but if true or false are being passed in directly then there is no point in using toggleClass. Its supposed to work with passing in a boolean variable that determines whether to add or remove. With true or false being passed in, I think performance is actually worse.


change = val.replace( new RegExp( "^" + lastval ) , "" );

if( val.length < lastval.length || change.length != ( val.length - lastval.length ) ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably should've asked this before I landed this code, but ... why can't we simply compare lastval against val?

if ( val !== lastval) {

Why do we have to do all this replace and and length checking business?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard my previous note (duh) ... I forgot you were trying to distinguish between appended characters which is part of the optimization.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants