-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Optimize the GeoJSON ⇢ worker transfer #1504
Comments
What about transferable objects? |
Although the transferred object would be removed from the main process? Not sure how you'd deal with that unless you want to deep clone the data before transfer. Maybe that's just as slow as stringify. I guess postMessage(object) is doing the fastest deep clone you could get :/ Maybe you could transfer into the worker and transfer back? heh |
That's another thing to try, although again, it would require a stringify + parse, plus additional overhead in copying bytes from a string to a typed array and back. |
Ya. Was later thinking that maybe one way to ensure great performance would be to get your ad hoc code into the worker somehow. |
Per chat with @jfirebaugh we should perf out a few different ways to move data to Worker processes before we try to improve this perf. He recommended looking into:
I'd like to dive into this, but I'm a few weeks out before I'd have the time to really solve this problem. |
In #2001, @dcervelli provided a PR that accepts pre-stringified GeoJSON, citing a 6x perf boost. We decided to take a step back and think about the best long term solution to the problem before including anything in the next release. @mourner suggested two additional transfer techniques we should investigate
|
@dcervelli @jfirebaugh @lucaswoj Just did a quick test of the stringified JSON approach. Performance on a 11MB GeoJSON in Chrome:
The first two would happen on the main thread, then sent as transferable to the worker. Converting back to string on the worker side is expensive, but at least it's not blocking the main thread. Geobuf would probably cope much better in this regard though. Also not sure how much less this blocks compared to structured cloning transfer of json — needs a benchmark. Test code: console.time('stringify');
var str = JSON.stringify(json);
console.timeEnd('stringify');
console.time('convert to Uint16Array');
var bytes = new Uint16Array(str.length);
for (var i = 0; i < str.length; i++) bytes[i] = str.charCodeAt(i);
console.timeEnd('convert to Uint16Array');
console.time('convert back to string');
var str2 = '';
for (var i = 0; i < bytes.length; i++) str2 += String.fromCharCode(bytes[i]);
console.timeEnd('convert back to string');
console.time('parse json');
var json2 = JSON.parse(str2);
console.timeEnd('parse json'); |
I'd like to add the "skipWorker" approach to the list of things we should test
|
Spent some time experimenting today. Results of my benchmarks for sending a 11MB JSON sample to a worker in Chrome: # structured cloning:
send: 3351ms
# transferable string bytes hack
pack: 235ms
send: 401ms
unpack: 612ms
roundtrip: 1249ms
# stringify/parse
pack: 162ms
send: 412ms
unpack: 183ms
roundtrip: 757ms
# transferable geobuf
pack: 89ms
send: 366ms
unpack: 132ms
roundtrip: 587ms Geobuf is the best option overall, with stringify/parse (without transferable) coming close. Big surprise here is the fact it sends a huge string in about the same time as doing an ownership transfer of an array buffer. Also, I really don't understand why structured cloning of JSON is so incredibly slow in Chrome. Now curious to check this stuff on Firefox and Safari. |
Firefox is even faster sending strings, and has proper transferable performance: # stringify/parse
pack: 190ms
send: 150ms
unpack: 171ms
# geobuf
pack: 147ms
send: 24ms
unpack: 182ms Now I wonder if there's a bug with transferable objects in the current Chrome. |
OK figured out the Chrome issue — I was instantiating a worker and then immediately sending a message to it, but it wasn't ready to immediately respond; if I do benchmarks on 1s timeout after worker, all browsers properly do buffer transfer in 0ms. That brings us to the following results in Chrome: # structured cloning:
send: 2385ms
# stringify/parse
pack: 170ms
send: 14ms
unpack: 173ms
roundtrip: 357ms
# transferable geobuf
pack: 89ms
send: 0ms
unpack: 132ms
roundtrip: 221ms |
For the record: also tried MessagePack, and my own Sax-style Protobuf-based JSON serialization. Both worse than stringify/parse and geobuf: # message pack
pack: 257ms
unpack: 200ms
# jsonbuf
pack: 341ms
unpack: 360ms |
Round-trip performance on very small GeoJSON sources is equally important to performance on very large GeoJSON sources. We should try to benchmark both. |
@lucaswoj sure, although performance depends linearly on size in this case, so I'm certain the results will generally be similar. |
The bottlenecks processing 11MB GeoJSON file are very different from those of a GeoJSON file with a dozen vertices. Having very fast processing of small GeoJSON files is important for gl-draw and other interactivity / animation use cases. |
That is to say, the linear-time costs might be superseded by some constant-time costs. |
@lucaswoj what would be a good test case for this? Sending a very simple GeoJSON (a few polys, lines and points drawn in geojson.io) there and back a 1000 times? |
That sounds like a good test case to me 👍 |
@lucaswoj @jfirebaugh made a benchmark for a relatively small (80kb) GeoJSON. It runs as follows:
Results:
|
Also tested a super-tiny GeoJSON, just a few simple features (5kb). This time Geobuf is pretty close to Stringify/Prase.
|
#1584 might help too! |
@lucaswoj that one probably has a minor difference, I wanted to fix this mainly so that it doesn't eat up errors. |
According to mapbox/mapbox-gl-js#1504 the transferring of GeoJSON files between main and worker threads can cause a bottleneck. We should be able to reduce this bottleneck by converting and passing the GeoJSON file created in the Worker as a geobuf and decoding it on the main thread. This commit is broken because of a possible bug with Webworkify not pulling in correct dependencies. See browserify/webworkify#14
Any progress on this? |
@nrathi do you have a particular problem with this? It's much faster with recent releases. |
I am trying to "animate" a 6.67 MB GeoJson and it's not great. Every 500ms, I create a new GeoJson on the client-side fro a pre-loaded GeoJson and JSON, which takes about 10-15ms, and call setData. About 1s (or more) later, the new GeoJson is finally drawn. I even drop frames as the animation progresses. Do you have any recommendations? Thanks in advance. |
Our GeoJSON transfer performance is much faster in recent releases. Let's ticket out future perf improvement concepts separately in the future. |
Of course, it's always best to load directly into the worker with an external URL, but many use cases with GeoJSON requiring generating / filtering / sorting GeoJSON on the main thread and then passing it to a GeoJSON layer. We could try using blob URLs to see if it helps.
On the main thread:
On the worker thread, do a normal ajax request and then
revokeObjectURL
.My guess is this won't really help because you still have to stringify the GeoJSON, but it's worth a shot.
The text was updated successfully, but these errors were encountered: