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

$.map() fails with large arrays in Сhrome #4320

Closed
fantaclaus opened this issue Mar 13, 2019 · 11 comments
Closed

$.map() fails with large arrays in Сhrome #4320

fantaclaus opened this issue Mar 13, 2019 · 11 comments

Comments

@fantaclaus
Copy link

When a large array is passed as an argument to $.map() we get an error:

RangeError: Maximum call stack size exceeded

Tested on Chrome 72 for Windows.
JQuery version: 3.3.1

Sample code:

$.map(new Array(300000), i => 1)

Internally it calls to concat.apply and crashes there.

As long as I understand, apply tries to push all elements of the array into the stack before invoking the function concat and fails on stack shortage.

I believe that any function being invoked thru apply with a large array as an argument will fail.

I've found about 14 usages of apply in the code of jQuery. I would suggest you to check out all of these cases.

@dmethvin
Copy link
Member

Applying $.map to 300,000 elements isn't a use case we've considered. Since we're a DOM library, the most common calls relate to arrays of DOM elements and it's not common to have that many DOM elements. Are there situations where the internal uses might be passed hundreds of thousands of elements?

In an earlier ticket you mentioned that this happened with a templating library. Instead of processing one big template perhaps you could process multiple templates in groups.

@fantaclaus
Copy link
Author

Well, in fact I've just got an error with 130,000 items...

Why do you think that it is uncommon? Someone could develop an intranet business application that work with large tables. A table easily could have about 6,500 rows with 10 columns, So you've got 65,000 cells. Each cell could contain two <span> tags, so you've got 130,000 tags...

Anyway, the documentation doesn't mention about any limitations. The function $.map is described like a common use function. The native Array.map doesn't crash on such arrays, so it is quite unexpected when the jQuery's one does.

In an earlier ticket you mentioned that this happened with a templating library. Instead of processing one big template perhaps you could process multiple templates in groups.

It was not my ticket.

I'm concerned that the problem is general for jQuery since it uses apply method internally. So users could have troubles with some other functions, not only $.map. And they really have (according to some closed issues).

@dmethvin
Copy link
Member

Nobody should be rendering 6,500 rows on a page simultaneously. That is a good case for pagination or other segmentation techniques like a virtual scrolling window.

So users could have troubles with some other functions, not only $.map. And they really have (according to some closed issues).

There are certainly occasional tickets where people hit these limits in jQuery, but as far as I can recall they have all been in this category of use cases that are unsupported and/or inadvisable.

Anyway, the documentation doesn't mention about any limitations.

It's not a limitation in jQuery, it's in Chrome. Firefox works up to at least 500,000. There's got to be a limit somewhere and using a design that hits a rarely-hit limit probably means you're pushing the limits in inadvisable ways.

@mgol
Copy link
Member

mgol commented Mar 14, 2019

I wouldn't be opposed to using native Array#flat when available in the current browser if that doesn't increase the bundle too much. We'd have to create an internal flat function that would defer to Array#flat when available and to array => concat.apply( [], array ) when not available and use it consistently around the code base. It might even be faster than our implementation, who knows?

This wouldn't help in IE (and Edge, but that will change soon) by default but:

  1. Some apps don't support IE.
  2. Some apps provide polyfill for new APIs; if their flat polyfill doesn't suffer from the limitation that using concat.apply does, it'd work even there.

I'll reopen so that we can discuss it a little more.

@mgol
Copy link
Member

mgol commented Mar 14, 2019

I've just noticed @gibson042's comment at #4318 about exactly this issue where he mentions more cases that'd need to be fixed if that's desired (while watching the size).

@Spazierenman
Copy link

Spazierenman commented Mar 15, 2019

I agree with @fantaclaus. The modern world does not stand still, data volumes, the sizes of pages and monitors grow. Perhaps $ .map () suits you, but your children will say who needs a function like this?

@timmywil
Copy link
Member

We haven't been able to spend time on jquery/sizzle#403, but we're thinking that solution can be exposed and used in jQuery to solve this issue.

@timmywil
Copy link
Member

Reminder: consider jQuery.merge when addressing this issue.

@timmywil timmywil added this to the Future milestone May 13, 2019
@aelafifi
Copy link
Contributor

I would like to help in resolving this issue.

@timmywil
Copy link
Member

@aelafifi Feel free to make a PR! Check out CONTRIBUTING.md if you need some guidance getting started.

@mgol
Copy link
Member

mgol commented Aug 19, 2019

PR: #4459

@mgol mgol modified the milestones: Future, 3.5.0 Sep 24, 2019
@mgol mgol closed this as completed in 9df4f1d Sep 25, 2019
mgol pushed a commit to mgol/jquery that referenced this issue Sep 25, 2019
Calling `Array.prototype.concat.apply( [], inputArray )` to flatten `inputArray`
crashes for large arrays; using `Array.prototype.flat` avoids these issues in
browsers that support it. In case it's necessary to support these large arrays
even in older browsers, a polyfill for `Array.prototype.flat` can be loaded.
This is already being done by many applications.

(cherry picked from 9df4f1d)

Fixes jquerygh-4320
Closes jquerygh-4459
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

6 participants