Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Rework map threading #879

Closed
wants to merge 43 commits into from
Closed

Rework map threading #879

wants to merge 43 commits into from

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Feb 13, 2015

This changes the way Map objects are being rendered. Instead of start() and stop() launching a new thread, we're now starting a new thread as soon as the Map object is constructed, which runs until the Map object is destructed. This means that we don't have to start/stop thread for static rendering and overall simplifies the way we access OpenGL from multiple threads (we don't anymore).

  • OpenGL initialization in Map thread
  • Handle HeadlessView resizes
  • Make stop() wait until rendering is finished
  • Guard OpenGL extension function pointer acquisition against race conditions (=> uv_once)
  • Terminate must terminate all pending requests from this map. This likely requires that the FileSource knows about Map object and keeps track of which Request objects belong to what Maps so we can cancel those automatically
  • Fix android builds
  • Get iOS backgrounding/foregrounding actions right
  • Disable fading for static map renders (see Disable fading when rendering still images #942)

@mb12
Copy link

mb12 commented Feb 19, 2015

I am unable to reason how the following scenario would work correctly in the new threading model.

1.) SourceInfo objects are created upon parsing style.json.
2.) TileData holds a reference to SourceInfo.
3.) TileParser holds a reference to TileData.

When style.json is changed these SourceInfo objects would get deleted. ( They reside under Map.style.layers.layers.bucket.style_source.info ).
If any of the TileParser workers has not finished after Map.style has been changed, this would cause an invalid access.

@mikemorris
Copy link
Contributor

Working with @ljbade to debug a deadlock on the std::recursive_mutex in mbgl::Transform when running the Android app (RenderMode::Continuous), seems to get blocked in Transform::resize. Digging through some of the suggestions in https://stackoverflow.com/questions/13755355/android-ndk-mutex-locking currently...

FWIW, running the app on OS X through LLDB is working fine. I'm wondering if this is somehow unique to the environment the app is running in on Android, like if the JVM is calling things from different threads than expected, similar to what we hit with map_thread asserts in Node.js and Android previously.

@ljbade
Copy link
Contributor

ljbade commented Feb 25, 2015

OK I set up some logging to print the mutex value before and after each lock in Transform.

Here is what happens during deadlock:

02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value before = 4000
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value after = 1ed74001
...
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ Transform::setzoom
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value before = 4000
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value after = 1ed74001
...
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ Transform::setangle
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value before = 4000
02-25 17:33:43.194    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value after = 1ed74001
...
02-25 17:33:43.334    7895-7895/com.mapbox.mapboxgl.app I/MapView﹕ resize 4 1080 1677
02-25 17:33:43.344    7895-7895/com.mapbox.mapboxgl.app D/JNI﹕ nativeResize
02-25 17:33:43.344    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ Map::resize
02-25 17:33:43.344    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ Transform::resize
02-25 17:33:43.344    7895-7895/com.mapbox.mapboxgl.app D/Android﹕ mtx value before = 7989d5e8

Something is corrupting the mutex value which results in a deadlock because it thinks some random thread is holding the lock.

@ljbade
Copy link
Contributor

ljbade commented Feb 25, 2015

I also logged the pointer value (that points to the mutex value). You can see the pointer is being changed, when it shouldn't so the value is just random memory.

02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value before = 4000
02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value after = 2ee04001
02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value before = 4000
02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value after = 2ee04001
02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value before = 4000
02-25 17:40:53.313  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b274, mtx value after = 2ee04001
02-25 17:40:53.473  12000-12000/com.mapbox.mapboxgl.app D/Android﹕ mtx pointer before = 7288b1f4, mtx value before = 7989e5e8

we are now scoping all file requests to an environment object. The FileSource implementation treats
this as an opaque pointer, but allows canceling all Requests that are associated with that pointer.
This is necessary to abort all file requests that originated from a particular Map object. Aborting
a file request is different from canceling a file request: A canceled request doesn't have its
callback called, while an aborted request will have its callback called with an error, indicating
that the environment is going to be shut down.
@kkaefer
Copy link
Contributor Author

kkaefer commented Mar 4, 2015

@mikemorris please note that the API of FileSource changed: 335cf4c. There's now an opaque Environment ref that is used to abort all requests originating from a Map. This is likely not very relevant for node-mapbox-gl-native since there's currently no way to abort a static map image render, but it needs to support the API nonetheless.

@kkaefer kkaefer mentioned this pull request Mar 4, 2015
@mikemorris
Copy link
Contributor

Pushed up some minimal changes in the environment branch to get everything compiling again. Is it acceptable to leave the NodeFileSource::abort method as a no-op and hold off on adding an Abort action, or should I work on building out those pieces too?

void cancel(Request *request) override;
void request(const Resource &resource, const Environment &env, Callback callback) override;

void abort(const Environment &env) override;

enum class CacheHint : uint8_t { Full, Refresh, No };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this moved to FileCache::Hint and is no longer used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, removed in 10d84f6.

@kkaefer
Copy link
Contributor Author

kkaefer commented Mar 12, 2015

Removing this from the milestone as all relevant patches have been integrated into master separately. Will circle back to this after iOS Beta release

@kkaefer kkaefer removed this from the iOS Beta milestone Mar 12, 2015
@mikemorris
Copy link
Contributor

Did something change with the Environment work to start emitting DEBUG severity level events even when built with BUILDTYPE=Release? I'm guessing that the EventEmitter with NodeLogBackend should be moved from the module itself to the individual node_mbgl::Map objects?

1ec5 and others added 3 commits March 19, 2015 17:40
No need to load higher-resolution vector tiles for HiDPI displays.

Fixes #907.
Do not try to fulfil the promise twice in case both requests to the sprite JSON and image fail. this will crash the program. Instead, we always continue with the promise, instead of throwing an exception. This allows the program to continue parsing tiles, but without an image sprite available. This means the map will render, but without the sprite images
@kkaefer
Copy link
Contributor Author

kkaefer commented Apr 20, 2015

This has been superseded by #1272

@kkaefer kkaefer closed this Apr 20, 2015
@kkaefer kkaefer deleted the static-render branch April 20, 2015 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants