Skip to content

Add "skipWorker" option to GeoJSON sources [DO NOT MERGE] - #1979

Closed
lucaswoj wants to merge 2 commits into
masterfrom
skip-worker
Closed

Add "skipWorker" option to GeoJSON sources [DO NOT MERGE]#1979
lucaswoj wants to merge 2 commits into
masterfrom
skip-worker

Conversation

@lucaswoj

Copy link
Copy Markdown
Contributor

Many users, especially mapbox-gl-draw, have demonstrated a need for high responsiveness in updating GeoJSON sources. One potential solution is to do GeoJSON parsing / tiling in the main thread rather than the worker.

mapbox/mapbox-gl-draw#185

Next Steps

  • quantify performance change
  • quantify bundle size change
  • DRY up code
  • fix featuresIn / featuresAt
  • add integration tests for GeoJSON sources (or ticket out separately)

@mcwhittemore

Copy link
Copy Markdown
Contributor

Assuming this is faster for X features and less, why make it a flag? Why not just always do it for that number of features?

I'm guessing number of features will be too simple. The size of the geometry might mater more. Either way, if this gets into gl-js, we should have a recommendation of when to use which mode.

@lucaswoj

Copy link
Copy Markdown
Contributor Author

Assuming this is faster for X features and less, why make it a flag? Why not just always do it for that number of features?

I'm guessing number of features will be too simple. The size of the geometry might mater more. Either way, if this gets into gl-js, we should have a recommendation of when to use which mode.

These are all great questions for which I don't yet have good answers. I think the answer will be one of:

  • "this is faster for X verticies or less"
  • "this has different performance characteristics that are well suited for the the animation of simple geometries."
  • "this is a bad idea"

@lucaswoj

Copy link
Copy Markdown
Contributor Author

ref #1180
ref mapbox/DEPRECATED-mapbox-gl#7

@lucaswoj

Copy link
Copy Markdown
Contributor Author

As an added bonus, this would also allow for easier integration testing (ref #1987 (comment))

@mourner

mourner commented Jan 27, 2016

Copy link
Copy Markdown
Member

@lucaswoj I'm 👍 on such an option (off by default, no smart heuristics for now). The main tricky thing here is how to DRY up the code. :) Will probably require moving out GeoJSON handling into a separate file.

@lucaswoj

Copy link
Copy Markdown
Contributor Author

My plan for DRYing this code is to refactor the Dispatcher and Actor to optionally run operations on the same thread instead of in a Worker.

@lucaswoj

Copy link
Copy Markdown
Contributor Author

We might be able to get similar performance characteristics using a transferrable byte array containing stringified GeoJSON or geobuf. #1504

@lucaswoj

Copy link
Copy Markdown
Contributor Author

Closing while we consider other solutions to this problem in #1504

@lucaswoj

lucaswoj commented Mar 8, 2016

Copy link
Copy Markdown
Contributor Author

#2224 is a much better solution than this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature 🍏 needs discussion 💬 performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants