Better separation between UI thread and Map thread

[kkaefer]

TODO list:

• data.callback is not threadsafe
• refactor View and subclasses to interact with a single thread only (either main or map)

[jfirebaugh]

How do you decide what goes in MapData and what goes in MapContext?

[kkaefer]

All data that is shared between the main thread and the map thread goes into MapData, the rest goes into MapContext. Eventually this should be 0, so MapData goes away and data lives either on the main thread (in the Map object) or on the map thread (in the MapContext object).

[jfirebaugh]

👍, sounds good.

[kkaefer]

This is more of a work in progress and not intended to be merged as is.

[jfirebaugh]

What tasks are remaining, in your mind?

[kkaefer]

Mostly removal of accessing MapContext from Map

[jfirebaugh]

I started looking at what needs to happen to privatize the remaining members of MapContext. The asyncs were the first target. They are currently public because they are used for control flow in the static rendering case. I think we should replace this with explicit checks of whether all resources are ready (i.e. port map.loaded() from gl-js and call the renderStill callback at the equivalent point to where gl-js emits the load event.

[jfirebaugh]

View and subclasses need to be refactored as a part of this split too. Right now they are a mix of code executed on the map thread and code executed on the rendering thread (via abstract functions called by MapContext).

[jfirebaugh]

    // Run the event loop once more to make sure our async delete handlers are called.
    uv_run(env.loop, UV_RUN_ONCE);

Is this for sure necessary?

[mikemorris]

@jfirebaugh As you're digging into this, I would love if you could keep an eye out for threading issues that could possibly be causing https://github.com/mapbox/api-gl/issues/16 and https://github.com/mapbox/node-mapbox-gl-native/issues/114?

[kkaefer]

// Run the event loop once more to make sure our async delete handlers are called.
uv_run(env.loop, UV_RUN_ONCE);

Is this for sure necessary?

Not 100% sure, but there could be situations where we end an event loop, and destroy it, but don't run the callbacks from uv_close(...).

[tmpsantos]

Why not call MapContext Map::Impl like we do for the other objects?

[jfirebaugh]

@mikemorris Will do.

Why not call MapContext Map::Impl like we do for the other objects?

Yeah, I was considering renaming it later on, when everything is working again.

[jfirebaugh]

Got it to compile for the first time using Thread<MapContext>. First crash is related to view functions calling back into map functions from the rendering thread:

#0  0x0000000100245e4f in void mbgl::util::Thread<mbgl::MapContext>::invoke<void (mbgl::MapContext::*)(mbgl::Update), mbgl::Update>(void (mbgl::MapContext::*)(mbgl::Update), mbgl::Update&&) at /Users/john/Development/mapbox-gl-native/src/mbgl/util/thread.hpp:47
#1  0x00000001002424bf in mbgl::Map::resize(unsigned short, unsigned short, float, unsigned short, unsigned short) at /Users/john/Development/mapbox-gl-native/src/mbgl/map/map.cpp:172
#2  0x00000001002fb160 in mbgl::View::resize(unsigned short, unsigned short, float, unsigned short, unsigned short) at /Users/john/Development/mapbox-gl-native/src/mbgl/map/view.cpp:24
#3  0x00000001004ffc73 in mbgl::HeadlessView::discard() at /Users/john/Development/mapbox-gl-native/platform/default/headless_view.cpp:212
#4  0x000000010050188a in mbgl::HeadlessView::activate() at /Users/john/Development/mapbox-gl-native/platform/default/headless_view.cpp:308
#5  0x0000000100267d2b in mbgl::MapContext::MapContext(uv_loop_s*, mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool) at /Users/john/Development/mapbox-gl-native/src/mbgl/map/map_context.cpp:56
#6  0x0000000100269184 in mbgl::MapContext::MapContext(uv_loop_s*, mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool) at /Users/john/Development/mapbox-gl-native/src/mbgl/map/map_context.cpp:62
#7  0x00000001002453de in void mbgl::util::Thread<mbgl::MapContext>::run<std::__1::tuple<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>, 0ul, 1ul, 2ul, 3ul>(std::__1::tuple<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>&&, (anonymous namespace)::index_sequence<0ul, 1ul, 2ul, 3ul>) at /Users/john/Development/mapbox-gl-native/src/mbgl/util/thread.hpp:120
#8  0x0000000100266213 in mbgl::util::Thread<mbgl::MapContext>::Thread<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, mbgl::Environment&&&, mbgl::View&&&, mbgl::MapData&&&, bool&&&)::'lambda'()::operator()() const at /Users/john/Development/mapbox-gl-native/src/mbgl/util/thread.hpp:107
#9  0x0000000100265e72 in std::__1::__invoke<mbgl::util::Thread<mbgl::MapContext>::Thread<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, mbgl::Environment&&&, mbgl::View&&&, mbgl::MapData&&&, bool&&&)::'lambda'()>(decltype(std::__1::forward<mbgl::util::Thread<mbgl::MapContext>::Thread<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, mbgl::Environment&&&, mbgl::View&&&, mbgl::MapData&&&, bool&&&)::'lambda'()>(fp)(std::__1::forward<>(fp0))), mbgl::util::Thread<mbgl::MapContext>::Thread<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, mbgl::Environment&&&, mbgl::View&&&, mbgl::MapData&&&, bool&&&)::'lambda'()&&) [inlined] at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__functional_base:413
#10 0x0000000100265e66 in _ZNSt3__116__thread_executeIZN4mbgl4util6ThreadINS1_10MapContextEEC1IJRNS1_11EnvironmentERNS1_4ViewERNS1_7MapDataERbEEERKNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEEDpOT_EUlvE_JEJEEEvRNS_5tupleIJT_DpT0_EEENS_15__tuple_indicesIJXspT1_EEEE [inlined] at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/thread:332
#11 0x0000000100265e56 in std::__1::__thread_proxy<std::__1::tuple<mbgl::util::Thread<mbgl::MapContext>::Thread<mbgl::Environment&, mbgl::View&, mbgl::MapData&, bool&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, mbgl::Environment&&&, mbgl::View&&&, mbgl::MapData&&&, bool&&&)::'lambda'()> >(void*, void*) at /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/thread:342
#12 0x00007fff8c8e0268 in _pthread_body ()
#13 0x00007fff8c8e01e5 in _pthread_start ()
#14 0x00007fff8c8de41d in thread_start ()

Going to start looking at how @kkaefer is restructuring this in transform-on-main.

[jfirebaugh]

What is the purpose of Environment::abandon* as opposed to immediately disposing the resources?

[jfirebaugh]

@kkaefer Can you give an outline of your strategy with transform-on-main?

[jfirebaugh]

@mikemorris @kkaefer Is it essential that Map::renderStill be asynchronous? It's not reentrant, and I imagine that node-mbgl is running the maps on a thread pool anyway, right?

[mb12]

Environment::abandonVAOs was added to make sure that buffers for DebugBucket get also deleted in the mapThread. Prior to this change, it used to result in "Opengl thread conflict" error.

#1016

[jfirebaugh]

Current status: GLFW-based app is working. Headless rendering hangs. iOS / Android are probably failing to build.

[mikemorris]

Is it essential that Map::renderStill be asynchronous? It's not reentrant, and I imagine that node-mbgl is running the maps on a thread pool anyway, right?

So I know there were problems with simply using NanAsyncWorker and a synchronous render call with Node's default threadpool, so the current architecture is to start the render on the main thread but actually perform work on the Map thread to not block Node.js. I think @kkaefer may be able to explain this better.

[kkaefer]

I'd like to keep renderStill asynchronous. Even if we'd offload the then blocking call to renderStill() to a libuv worker thread, it's still blocking which isn't something that you can do with libuv. It'd mean that the libuv threadpool is blocked by wait calls, which means we couldn't do any IO either anymore (i.e. we can't satisfy the dependencies the running render call needs), resulting in a deadlock.

[jfirebaugh]

I'm still chasing a crash in the headless tests on Travis (can't reproduce locally), but it doesn't seem to manifest anywhere else and we need to get this into wider testing. I'm going to disable the failing tests temporarily, merge the branch, and then resume banging my head against the wall.

[bleege]

it doesn't seem to manifest anywhere else and we need to get this into wider testing. I'm going to disable the failing tests temporarily, merge the branch, and then resume banging my head against the wall.

👍 We can roll a new Sirius later today to get this into wider circulation ASAP.

[jfirebaugh]

Merged! Closing a bunch of issues that are known to be fixed or likely to be fixed by these changes. Please open new issues for any crashers observed post-merge.

[incanus]

Let’s get this into a build today along with whatever else we can scare up, code or design. /cc @samanpwbb @peterqliu @nickidlugash @andreasviglakis