[select] use for loop instead of `push.apply`

[timmywil]

Migrated from https://github.com/jquery/jquery/issues/3704

We decided in the meeting to try a for loop instead of push.apply. Check perf to be sure, but for loops are so heavily optimized these days that it might even be better. It has the added advantage of not causing "stack limit exceeded" errors.

[Krinkle]

@timmywil If perf does become an issue and you find a significant difference with large work loads, another compromise might be to batch it.

At Wikimedia, variations of the following utility exist in a few larger projects. https://github.com/wikimedia/VisualEditor/blob/bb4863019/src/ve.utils.js#L365-L393

/**
 * Push one array into another.
 *
 * This is the equivalent of arr.push( d1, d2, d3, ... ) except that arguments are
 * specified as an array rather than separate parameters.
 *
 * @param {Array|ve.dm.BranchNode} arr Object supporting .push(). Will be modified
 * @param {Array} data Array of items to insert.
 * @return {number} Length of the new array
 */
ve.batchPush = function ( arr, data ) {
    // We need to push insertion in batches, because of parameter list length limits which vary
    // cross-browser - 1024 seems to be a safe batch size on all browsers
    var length,
        index = 0,
        batchSize = 1024;
    if ( batchSize >= data.length ) {
        // Avoid slicing for small lists
        return arr.push.apply( arr, data );
    }
    while ( index < data.length ) {
        // Call arr.push( i0, i1, i2, ..., i1023 );
        length = arr.push.apply(
            arr, data.slice( index, index + batchSize )
        );
        index += batchSize;
    }
    return length;
};

[NN---]

Any update when this issue will be fixed ?

[gibson042]

https://jsperf.com/arraylike-push-many

With array-like objects, push(_, NodeList) and the simple assignment loop emerged as clear winners on Chrome, Firefox, Safari, Edge, iOS, and Android (and the other push(_, slice) approaches benefited from large batch sizes). On IE10 and IE11, it was push(_, NodeList) by itself and simple assignment was mid-pack (but only with a 10–20% hit, comparable to 100-item batches). It was a full 33% slower on IE9, but I think I can live with that.

When the receiver is an array instead of an ordinary object, though, Firefox, Edge, and sometimes IE penalize simple assignment by up to 20%, and Safari's performance drops by an order of magnitude.

So the only consistently good option is push(_, NodeList), which throws exceptions when adding too. To be honest, I'm extremely conflicted, but will probably switch to a simple assignment loop and hope WebKit gets faster at some point.

[kjhangiani]

Has there been any update / movement on this issue? Seeing these errors occur randomly throughout our app, and tracing the error led me to this particular issue in Sizzle

[mgol]

@gibson042

https://jsperf.com/arraylike-push-many

This test case is 404 now, do you have a copy?

[mgol]

There's a jQuery PR for this issue now: https://github.com/jquery/jquery/pull/4459

[JoolsCaesar]

Any movement on this? It's still causing "stack limit exceeded" errors in the latest versions of jquery e.g. from 3.6.0:

                    // If seed is empty or no tokens remain, we can return early
                    tokens.splice( i, 1 );
                    selector = seed.length && toSelector( tokens );
                    if ( !selector ) {
                        **push.apply( results, seed );**
                        return results;
                    }

[mgol]

jQuery version at https://github.com/jquery/jquery/issues/5298