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

When using a Filter with lots of elements, get "Uncaught RangeError: Maximum call stack size exceeded" #1782

Closed
peckjon opened this issue Dec 2, 2015 · 13 comments
Assignees

Comments

@peckjon
Copy link
Contributor

peckjon commented Dec 2, 2015

I'm attempting to render a Layer with a Filter statement containing a few thousand elements. Example:

var districtIds = []; i=1; while(districtIds.push(i++)<3000);
var idFilter = ["in","district_id"].concat(districtIds);
console.log("idFilter:");
console.log(idFilter);
map.addLayer({
    "id": "district_shape",
    "interactive": true,
    "type": "line",
    "source": "district_group",
    "source-layer": "district_shape",
    "filter": idFilter,
    "layout": {
        "line-join": "round",
        "line-cap": "round"
    },
    "paint": {
        "line-color": "#794044",
        "line-width": 2
    }
});

If I omit the Filter statement, or decrease the size of districtIds to a few hundred elements, things work fine. But if I have a large number of elements in the Filter, this happens:

Uncaught RangeError: Maximum call stack size exceeded
  Function @ native v8natives.js:1261
  module.exports @ mapbox-gl-dev.js:15612
  Bucket @ mapbox-gl-dev.js:64
  LineBucket @ mapbox-gl-dev.js:936
  Bucket.create @ mapbox-gl-dev.js:26
  WorkerTile.parse @ mapbox-gl-dev.js:6377
  done @ mapbox-gl-dev.js:6229
  xhr.onload @ mapbox-gl-dev.js:12895

I've not had any luck finding a workaround... it appears to be a fairly low-level bug having to do with how/when the Filter function is compiled.

I can provide a (non-)working example if needed.

Thanks,
Jon Peck
jon@empowerengine.com

@peckjon
Copy link
Contributor Author

peckjon commented Dec 2, 2015

@mourner : I've been told this might be in your purview?

@mourner
Copy link
Member

mourner commented Dec 2, 2015

This looks pretty fun, the filter system was not designed to handle few thousand rules in one place... Do you really have to filter this way, or could you add some single flag on the data side instead?

@drewbo
Copy link
Contributor

drewbo commented Dec 2, 2015

I have this same error using high numbers of filters. Shown in an unrelated issue example #1785 (the gist will give the error at zoom level 2)

@peckjon
Copy link
Contributor Author

peckjon commented Dec 2, 2015 via email

@diogobif
Copy link

I have this problem adding markers on a heat layer, and there was only 5 itens.

@mourner
Copy link
Member

mourner commented Dec 21, 2015

@diogobif can you put up a minimal JSFiddle test case that reproduces the issue?

@diogobif
Copy link

I'm running more tests now, if it persists, I'll send you my code!

@jfirebaugh
Copy link
Contributor

@diogobif It looks like you're using Mapbox.js, not Mapbox GL JS, so your issue is unrelated to this one.

@peckjon
Copy link
Contributor Author

peckjon commented Dec 27, 2015

@mourner I think I have a fix, but I'm not sure if it's in keeping with the codebase's general style, so please take a look and apply it if you feel it passes muster. Basically, I want to make it possible to specify the filter as a Function. Since this leads to cloning issues down the line, it actually needs to be passed as a String. And, of course, it can't use anything from the calling Scope. The change is quite simple, but is in https://github.com/mapbox/feature-filter:

function compile(filter) {
    if (typeof filter === "string") {return filter+'(p,t)';}
    return operators[filter[0]].apply(filter, filter);
}

Then, instead of specifying a Filter like so:

var idFilter = ["in", "district_id"].concat(districtIds);

One could optionally do the following:

var idFilter = "function(properties,type) {return ["+districtIds.join()+"].indexOf(properties.district_id)>=0;}";

While I dislike passing eval()d Strings, this approach would allow for much more flexibility, and also solves the problem at hand. I've done some basic testing of the approach, and it appears to run in similar time, and cause no other problems.

@peckjon
Copy link
Contributor Author

peckjon commented Dec 27, 2015

PS a possible change to avoid user-specified Functions as Strings: allow filter to be specified as Function, convert to String before cloning... however, this makes it somewhat less obvious that Scope needs to be obeyed.

@mourner
Copy link
Member

mourner commented Dec 27, 2015

@peckjon no, functions are not serializable. We have to keep the style spec fully JSON-compliant.

Could you set up a simple minimal JSFiddle test case that demonstrate the original issue (call stack overflow)?

@peckjon
Copy link
Contributor Author

peckjon commented Dec 27, 2015

@mourner https://jsfiddle.net/cvmcj9nk/ (note the console log)

If String-ified functions are abhorrent, this could also be solved by making the 'in' operation in feature-filter more efficient at https://github.com/mapbox/feature-filter/blob/master/index.js#L35

As of now, it ORs a set of equality checks instead of, for example, using .indexOf() on an Array. It is the size of that evaluated "(foo==bar)||..." String which causes the call stack exceeded error, at https://github.com/mapbox/feature-filter/blob/master/index.js#L77

peckjon added a commit to peckjon/feature-filter that referenced this issue Jan 12, 2016
peckjon added a commit to peckjon/feature-filter that referenced this issue Jan 13, 2016
Fixes mapbox/mapbox-gl-js#1782

If too many elements (varies by environment, but appx 1-2k) are used for an 'in' filter, it will fail with "Uncaught RangeError: Maximum call stack size exceeded". This is because the logical comparison of many infix comarisons (eg "(p['foo']==='bar') || (p['foo']==='cat') || ...") becomes too long for the v8natives to handle.
@peckjon
Copy link
Contributor Author

peckjon commented Jan 13, 2016

Pending fix at mapbox/feature-filter#7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants