Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mixin _.pushAll #3767

Merged
merged 1 commit into from
May 7, 2015
Merged

mixin _.pushAll #3767

merged 1 commit into from
May 7, 2015

Conversation

spalger
Copy link
Contributor

@spalger spalger commented May 6, 2015

This simple mixin efficiently copies the elements of one array into another.

Why?

While testing a bug that we suspected was caused by massive mappings, I created a mapping with 60,000 fields that crashed Kibana. It didn't run out of memory, or become unresponsive, it exceeded the maximum call stack by doing:

arr.push.apply(arr, otherArray);

This takes advantage of the multiple argument syntax of array push to push all of the elements from one array into another. Unfortunately it requires that all of the array elements are passed as function arguments which fills up the stack when the otherArray contains too many items.

@trevan
Copy link
Contributor

trevan commented May 6, 2015

In Firefox nightly, the apply.push is fastest, then forEach, then the allocate/fill, and finally splice.

@w33ble
Copy link
Contributor

w33ble commented May 6, 2015

Looks good to me. Was this case in segmented.js? I wanted to try testing it with larger objects.

@w33ble
Copy link
Contributor

w33ble commented May 7, 2015

I ran this against 2 400k long arrays with deep objects full of random data, and while it took a really long time to run, it ran just fine. So, I'm merging this. 😋

w33ble added a commit that referenced this pull request May 7, 2015
@w33ble w33ble merged commit 16c0dbd into elastic:master May 7, 2015
@spalger spalger deleted the mixin/pushAll branch July 31, 2015 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants