Allow GeoJSON to be passed as stringify JSON.

[dcervelli]

Mitigation for #1504.

With a complex GeoJSON object (say 20k features with 100k coordinates) the postMessage call to the worker takes several seconds on the UI thread. By manually JSON.stringifying the GeoJSON object and JSON.parseing on the worker side we're seeing a 6x perf improvement.

Ideally we would figure out a way to use transferables or similar (there is still a small UI freeze in our usage), but this is a step in the right direction.

[lucaswoj]

Awesome idea! Thanks @dcervelli!

Can you add some unit tests and fix the lint errors showing up on CircleCI (or locally by running npm test)?

Thoughts @mourner?

[dcervelli]

Fixed up the lint errors and added a test.

Also, FWIW, I think the test in geojson_source.test.js:146 doesn't do anything. As far as I could tell source._pyramid.renderedIDs() never changes from [].

[lucaswoj]

ref #762

[mourner]

I'm surprised it's 6 times faster but 👍 on the change. @dcervelli just wondering, are you changing all the features in the GeoJSON real-time that makes it impossible to load through an url?

[lucaswoj]

Also, FWIW, I think the test in geojson_source.test.js:146 doesn't do anything. As far as I could tell source._pyramid.renderedIDs() never changes from [].

Yeah, our GeoJSONSource test doesn't actually exercise the Worker so it is a little weak. I'm hoping to fix that with #1979.

[lucaswoj]

Merged as f9afff0

Thanks again @dcervelli! I really appreciate the PR!

[jfirebaugh]

Hi all, sorry I didn't chime in here earlier. @dcervelli, really appreciate the contribution but I have a concern about this approach.

I'm not crazy about widening the GeoJSONSource API such that a string input can be either a URL or stringified JSON. What I had in mind for #1504 was that you'd still pass a GeoJSON object, and the implementation would JSON.stringify on the way to the worker.

Passing stringified JSON might let you avoid one string ⇢ object ⇢ string roundtrip. But does it really, in practice? That would mean you never need the GeoJSON in object format on the main thread, which if that's the case, you're probably better off passing a URL anyway.

Thoughts?

[dcervelli]

@mourner Yes, our use case is very much around mapbox-gl-js being the view layer on a very dynamic app where feature sets and styling will be changing quite often. (The same use case is driving this PR: mapbox/feature-filter#9). A couple of years ago we hosted some Mapbox folks to show them some of our geospatial workflows and this is similar to those.

@lucaswoj You're very welcome! Thanks for the quick merge.

@jfirebaugh Maybe I took the easy way out here but this seemed like the safest change. I could imagine there are GeoJSONs where it's faster to go natively/doesn't matter. Basically I didn't want to have to convince myself that stringify-ing was always better/correct. In particular JSON.stringify is fundamentally different than postMessage's structured clone algorithm, a wholesale change seems too likely to result in edge-case regressions.

Also, to your second point. Honestly we don't want to store GeoJSON in object format but it's the path of least resistance to getting geo data from our app into mapbox-gl-js. The surface area of the change required to add a new source that super-efficiently transfers (i.e. transferables) a very memory-efficient geo data format is pretty big. If you guys are interested in taking such a change, there's a good chance we'd be up for developing it.

[mourner]

he surface area of the change required to add a new source that super-efficiently transfers (i.e. transferables) a very memory-efficient geo data format is pretty big. If you guys are interested in taking such a change, there's a good chance we'd be up for developing it.

That actually led me to a thought that https://github.com/mapbox/geobuf fits perfectly for the use case, because its encoding/decoding is about the same or faster than JSON.parse/stringify, AND it can be transferred immediately as Uint8Array. This would also make hacks like skipWorker unnecessary.

@lucaswoj @jfirebaugh @ansis thoughts? This would be really awesome application of geobuf, and I can make a quick proof of concept.

P.S. Thinking about this further, a stringified JSON can also be represented as a typed array to make it transferrable in theory. Should we try to do it?

[lucaswoj]

Sounds like there's more discussion to be had here. Lets revert this commit for the time being and move all discussion to #1504

[lucaswoj]

reverted in b5c52e1

[tikhonbelousko]

When are you guys planning to release this thing? My UI is blocking for 4 seconds right now with 90k lines in my GeoJSON object. 😬

[mourner]

@dazzz likely next week, along with some big changes like #2224. How does the master branch work for you? Is it noticeably better?

[tikhonbelousko]

@mourner In master postMessage works almost instantly: 25ms instead of 1300ms for v0.15.0. UI is not blocking anymore. Just wow!