[select] use for loop instead of push.apply

Migrated from jquery/jquery#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.

@timmywil

@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;
};

Any update when this issue will be fixed ?

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.

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

@gibson042

@gibson042

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

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

There's a jQuery PR for this issue now: jquery/jquery#4459

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;
					}

jQuery version at jquery/jquery#5298

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[select] use for loop instead of push.apply #403

Description

Activity

Krinkle commented on Aug 5, 2017

NN--- commented on Aug 13, 2017

gibson042 commented on Sep 5, 2017

kjhangiani commented on Feb 9, 2018

mgol commented on Aug 19, 2019

mgol commented on Aug 19, 2019

JoolsCaesar commented on Jul 22, 2021

mgol commented on Sep 7, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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