coalesce buffer deletions and run them in bulk during rendering

On iOS, the main thread does framebuffer operations just before it requests a rerender of the view contents. This means that GL operations on the Map thread will interfere with that and cause a crash. While most GL operations happen throughout rendering (which is synchronized with the main thread), deletion of shared_ptrs release in turn their OpenGL objects. This may happen at any time and isn't synced with rendering.

This patch changes this so instead of deleting all objects immediately, we are collecting all abandoned IDs and delete them in bulk once the main thread has given us permission to use OpenGL calls (i.e. during render()).

4 files changed, 107 insertions, 15 deletions

diff --git a/src/mbgl/map/environment.cpp b/src/mbgl/map/environment.cpp
index 1aea5aa0c9..469790501c 100644
--- a/src/mbgl/map/environment.cpp
+++ b/src/mbgl/map/environment.cpp
@@ -1,5 +1,6 @@
 #include <mbgl/map/environment.hpp>
 #include <mbgl/storage/file_source.hpp>
+#include <mbgl/platform/gl.hpp>
 
 #include <uv.h>
 
@@ -76,12 +77,12 @@ ThreadInfoStore threadInfoStore;
 
 } // namespace
 
-Environment::Scope::Scope(Environment& env, ThreadType type, const std::string& name)
+EnvironmentScope::EnvironmentScope(Environment& env, ThreadType type, const std::string& name)
     : id(std::this_thread::get_id()) {
     threadInfoStore.registerThread(&env, type, name);
 }
 
-Environment::Scope::~Scope() {
+EnvironmentScope::~EnvironmentScope() {
     assert(id == std::this_thread::get_id());
     threadInfoStore.unregisterThread();
 }
@@ -90,6 +91,12 @@ Environment::Environment(FileSource& fs)
     : id(makeEnvironmentID()), fileSource(fs), loop(uv_loop_new()) {
 }
 
+Environment::~Environment() {
+    assert(abandonedVAOs.empty());
+    assert(abandonedTextures.empty());
+    assert(abandonedBuffers.empty());
+}
+
 Environment& Environment::Get() {
     Environment* env = threadInfoStore.getThreadInfo().env;
     assert(env);
@@ -129,6 +136,50 @@ void Environment::cancelRequest(Request* req) {
     fileSource.cancel(req);
 }
 
+// #############################################################################################
+
+#pragma mark - OpenGL cleanup
+
+void Environment::abandonVAO(uint32_t vao) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedVAOs.emplace_back(vao);
+}
+
+void Environment::abandonBuffer(uint32_t buffer) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedBuffers.emplace_back(buffer);
+}
+
+void Environment::abandonTexture(uint32_t texture) {
+    assert(currentlyOn(ThreadType::Map));
+    abandonedTextures.emplace_back(texture);
+}
+
+// Actually remove the objects we marked as abandoned with the above methods.
+void Environment::performCleanup() {
+    assert(currentlyOn(ThreadType::Map));
+
+    if (!abandonedVAOs.empty()) {
+        MBGL_CHECK_ERROR(gl::DeleteVertexArrays(static_cast<GLsizei>(abandonedVAOs.size()),
+                                                abandonedVAOs.data()));
+        abandonedVAOs.clear();
+    }
+
+    if (!abandonedTextures.empty()) {
+        MBGL_CHECK_ERROR(glDeleteTextures(static_cast<GLsizei>(abandonedTextures.size()),
+                                          abandonedTextures.data()));
+        abandonedTextures.clear();
+    }
+
+    if (!abandonedBuffers.empty()) {
+        MBGL_CHECK_ERROR(glDeleteBuffers(static_cast<GLsizei>(abandonedBuffers.size()),
+                                         abandonedBuffers.data()));
+        abandonedBuffers.clear();
+    }
+}
+
+// #############################################################################################
+
 void Environment::terminate() {
     fileSource.abort(*this);
 }
diff --git a/src/mbgl/map/environment.hpp b/src/mbgl/map/environment.hpp
index b631abf13d..1cd33a5e6d 100644
--- a/src/mbgl/map/environment.hpp
+++ b/src/mbgl/map/environment.hpp
@@ -6,6 +6,7 @@
 
 #include <thread>
 #include <functional>
+#include <vector>
 
 typedef struct uv_loop_s uv_loop_t;
 
@@ -25,16 +26,8 @@ enum class ThreadType : uint8_t {
 
 class Environment final : private util::noncopyable {
 public:
-    class Scope final {
-    public:
-        Scope(Environment&, ThreadType, const std::string& name);
-        ~Scope();
-
-    private:
-        std::thread::id id;
-    };
-
     Environment(FileSource&);
+    ~Environment();
 
     static Environment& Get();
     static bool inScope();
@@ -42,10 +35,27 @@ public:
     static std::string threadName();
 
     unsigned getID() const;
+
+    // #############################################################################################
+
+    // File request APIs
     void requestAsync(const Resource&, std::function<void(const Response&)>);
     Request* request(const Resource&, std::function<void(const Response&)>);
     void cancelRequest(Request*);
 
+    // #############################################################################################
+
+    // Mark OpenGL objects for deletion
+    void abandonVAO(uint32_t vao);
+    void abandonBuffer(uint32_t buffer);
+    void abandonTexture(uint32_t texture);
+
+    // Actually remove the objects we marked as abandoned with the above methods.
+    // Only call this while the OpenGL context is exclusive to this thread.
+    void performCleanup();
+
+    // #############################################################################################
+
     // Request to terminate the environment.
     void terminate();
 
@@ -53,10 +63,24 @@ private:
     unsigned id;
     FileSource& fileSource;
 
+    // Stores OpenGL objects that we marked for deletion
+    std::vector<uint32_t> abandonedVAOs;
+    std::vector<uint32_t> abandonedBuffers;
+    std::vector<uint32_t> abandonedTextures;
+
 public:
     uv_loop_t* const loop;
 };
 
+class EnvironmentScope final {
+public:
+    EnvironmentScope(Environment&, ThreadType, const std::string& name);
+    ~EnvironmentScope();
+
+private:
+    std::thread::id id;
+};
+
 }
 
 #endif
diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp
index 22e6cbcebb..ae1cfd20f8 100644
--- a/src/mbgl/map/map.cpp
+++ b/src/mbgl/map/map.cpp
@@ -61,6 +61,7 @@ using namespace mbgl;
 
 Map::Map(View& view_, FileSource& fileSource_)
     : env(util::make_unique<Environment>(fileSource_)),
+      scope(util::make_unique<EnvironmentScope>(*env, ThreadType::Main, "Main")),
       view(view_),
       transform(view_),
       fileSource(fileSource_),
@@ -80,15 +81,28 @@ Map::~Map() {
         stop();
     }
 
+    // Extend the scope to include both Main and Map thread types to ease cleanup.
+    scope.reset();
+    scope = util::make_unique<EnvironmentScope>(
+        *env, static_cast<ThreadType>(static_cast<uint8_t>(ThreadType::Main) |
+                                      static_cast<uint8_t>(ThreadType::Map)),
+        "MapandMain");
+
     // Explicitly reset all pointers.
     activeSources.clear();
     sprite.reset();
     glyphStore.reset();
     style.reset();
-    texturePool.reset();
     workers.reset();
+    painter.reset();
+    annotationManager.reset();
+    lineAtlas.reset();
+    spriteAtlas.reset();
+    glyphAtlas.reset();
 
     uv_run(env->loop, UV_RUN_DEFAULT);
+
+    env->performCleanup();
 }
 
 uv::worker &Map::getWorker() {
@@ -113,7 +127,6 @@ void Map::start(bool startPaused) {
 
         // Remove all of these to make sure they are destructed in the correct thread.
         style.reset();
-        workers.reset();
         activeSources.clear();
 
         terminating = true;
@@ -233,7 +246,7 @@ void Map::run() {
         threadName += "andMain";
     }
 
-    Environment::Scope scope(*env, threadType, threadName);
+    EnvironmentScope mapScope(*env, threadType, threadName);
 
     if (mode == Mode::Continuous) {
         checkForPause();
@@ -749,6 +762,10 @@ void Map::prepare() {
 
 void Map::render() {
     assert(Environment::currentlyOn(ThreadType::Map));
+
+    // Cleanup OpenGL objects that we abandoned since the last render call.
+    env->performCleanup();
+
     assert(painter);
     painter->render(*style, activeSources,
                     state, animationTime);
diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp
index aed182671b..1832bd8f0c 100644
--- a/src/mbgl/map/tile_data.cpp
+++ b/src/mbgl/map/tile_data.cpp
@@ -97,7 +97,7 @@ void TileData::reparse(uv::worker& worker, std::function<void()> callback)
     new uv::work<util::ptr<TileData>>(
         worker,
         [this](util::ptr<TileData>& tile) {
-            Environment::Scope scope(env, ThreadType::TileWorker, "TileWorker_" + tile->name);
+            EnvironmentScope scope(env, ThreadType::TileWorker, "TileWorker_" + tile->name);
             tile->parse();
         },
         [callback](util::ptr<TileData>&) {