Fix #12959: Optimize library-wide patterns #1046

Merged
merged 1 commit into from Nov 26, 2012

7 participants

@gibson042
jQuery Foundation member

Sizes - compared to master @ 53a0666

    264563       (-36)  dist/jquery.js                                         
     92213      (-190)  dist/jquery.min.js                                     
     32661      (-142)  dist/jquery.min.js.gz
@gibson042
jQuery Foundation member

@jquerybot retest

@jaubourg
jQuery Foundation member

FMOI, What are the patterns and why do they end up being better?

@scottgonzalez
jQuery Foundation member

It'd be good to get these into our style guide so that everyone knows to follow them in the future.

For example, should we always be doing for loops in the form for ( ; i < len; i++ ) { ?

@rwaldron
jQuery Foundation member

The style guide actually explicitly states:

"string".match() is no longer used.

...despite it being used twice in src/event.js. The rationale, IIRC, is reducing the context shifts by only using RegExp API for RegExp operations and String API for String operations.

@dmethvin
jQuery Foundation member

"string".match(/pattern/g) has no analog in .exec(), which is why it was used there.

@rwaldron
jQuery Foundation member

The pattern being matched is just /\S+/g, wouldn't split be sufficient?

@dmethvin
jQuery Foundation member

Not that I know of. http://jsfiddle.net/SbRL4/

@jaubourg
jQuery Foundation member

I like the fact you don't need to trim when using match.

@gibson042
jQuery Foundation member
  • In regex (which started this whole thing), the main thing was unnecessary escaping of + and especially - to avoid jshint warnings (resolved by regexdash: true, which allows dash at the end of a character set)
  • There was also a bit of rearranging to put like with like (gzip does well on long similar strings in close proximity) and get some repeats of common disjunctions like input|select|textarea
  • The switch from .split to .match was motivated by a desire to abstract away the reventTypes introduced in 3fce794, and just ends up better IMO
  • With loops, we definitely like for ( ; i < len; i++ ) { and I'd like to add it to the style guide (one motivation for http://bugs.jquery.com/ticket/12960)

Other than that, there was just a little bit of cruft to cleanup.

@dmethvin
jQuery Foundation member

The changes look good to me in general, the -142 seems magical. Glad you found regexdash, it seems like jshint has an option for every occasion and that one was annoying.

I'm not a big fan of initializing a loop variable far away from where it's being used, but the "far away" part is more due to some of the huge complex methods we have. @mikesherov has been threatening to turn on some of the complexity metrics.

@timmywil
jQuery Foundation member

I asked John why that was in the styleguide and he said the point was originally the difference in null value management...

null.match(...); // => error
/\s/.exec( null ); // => `null`

However, as Dave pointed out, there is one thing you can't duplicate with exec and that is a global match. This can be very useful and Sizzle also takes advantage of this use of match. I still prefer exec in most cases.

@mikesherov
jQuery Foundation member
@gibson042
jQuery Foundation member

For the record, our other common numeric iterations look like:

  • while ( i-- ) {
  • while ( (scalar = array[i++]) ) {
  • for ( ; (elem = elems[i]) != null; i++ ) { (note redundancy with the preceding)

And on node iterations:

  • while ( (node = node.DOMRelation) ) {; while ( (node = node[ dir ]) ) {
  • for ( ; cur; cur = cur.DOMRelation ) {

With lots of deviations from all of the above all over the place. To be honest, I think there's a lot more savings to be had from reducing the amount of variation here.

@gibson042 gibson042 merged commit 0766240 into jquery:master Nov 26, 2012
@gibson042 gibson042 referenced this pull request in jquery/jquery-ui Apr 12, 2013
Closed

Code Review for the classes changes #790

@markelog markelog referenced this pull request Apr 16, 2013
@rwaldron rwaldron Fixes #13779. Remove nodes in document order (uses for loop matching …
…empty()).

Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
332ebeb
@markelog markelog pushed a commit to markelog/sizzle that referenced this pull request Dec 25, 2013
@gibson042 gibson042 No ticket: small size optimizations. c04fce3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment