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()).

9 files changed, 121 insertions, 32 deletions

diff --git a/src/mbgl/geometry/buffer.hpp b/src/mbgl/geometry/buffer.hpp
index 3649574bbf..4198425ecf 100644
--- a/src/mbgl/geometry/buffer.hpp
+++ b/src/mbgl/geometry/buffer.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/util/noncopyable.hpp>
+#include <mbgl/map/environment.hpp>
 
 #include <cstdlib>
 #include <cassert>
@@ -21,7 +22,7 @@ public:
     ~Buffer() {
         cleanup();
         if (buffer != 0) {
-            MBGL_CHECK_ERROR(glDeleteBuffers(1, &buffer));
+            Environment::Get().abandonBuffer(buffer);
             buffer = 0;
         }
     }
diff --git a/src/mbgl/geometry/line_atlas.cpp b/src/mbgl/geometry/line_atlas.cpp
index 8be1a8d53c..507cc7b087 100644
--- a/src/mbgl/geometry/line_atlas.cpp
+++ b/src/mbgl/geometry/line_atlas.cpp
@@ -1,3 +1,4 @@
+#include <mbgl/map/environment.hpp>
 #include <mbgl/geometry/line_atlas.hpp>
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/platform/log.hpp>
@@ -18,9 +19,10 @@ LineAtlas::LineAtlas(uint16_t w, uint16_t h)
 LineAtlas::~LineAtlas() {
     std::lock_guard<std::recursive_mutex> lock(mtx);
 
-    MBGL_CHECK_ERROR(glDeleteTextures(1, &texture));
-    delete[] data;
+    Environment::Get().abandonTexture(texture);
     texture = 0;
+
+    delete[] data;
 }
 
 LinePatternPos LineAtlas::getDashPosition(const std::vector<float> &dasharray, bool round) {
diff --git a/src/mbgl/geometry/sprite_atlas.cpp b/src/mbgl/geometry/sprite_atlas.cpp
index dce772f2e4..bf31c6e38e 100644
--- a/src/mbgl/geometry/sprite_atlas.cpp
+++ b/src/mbgl/geometry/sprite_atlas.cpp
@@ -1,3 +1,4 @@
+#include <mbgl/map/environment.hpp>
 #include <mbgl/geometry/sprite_atlas.hpp>
 #include <mbgl/platform/gl.hpp>
 #include <mbgl/platform/log.hpp>
@@ -293,8 +294,7 @@ void SpriteAtlas::bind(bool linear) {
 
 SpriteAtlas::~SpriteAtlas() {
     std::lock_guard<std::recursive_mutex> lock(mtx);
-
-    MBGL_CHECK_ERROR(glDeleteTextures(1, &texture));
+    Environment::Get().abandonTexture(texture);
     texture = 0;
     ::operator delete(data), data = nullptr;
 }
diff --git a/src/mbgl/geometry/vao.cpp b/src/mbgl/geometry/vao.cpp
index 00976b4d54..fef74396a9 100644
--- a/src/mbgl/geometry/vao.cpp
+++ b/src/mbgl/geometry/vao.cpp
@@ -1,6 +1,7 @@
 #include <mbgl/geometry/vao.hpp>
 #include <mbgl/platform/log.hpp>
 #include <mbgl/util/string.hpp>
+#include <mbgl/map/environment.hpp>
 
 namespace mbgl {
 
@@ -11,7 +12,7 @@ VertexArrayObject::~VertexArrayObject() {
     if (!gl::DeleteVertexArrays) return;
 
     if (vao) {
-        MBGL_CHECK_ERROR(gl::DeleteVertexArrays(1, &vao));
+        Environment::Get().abandonVAO(vao);
     }
 }
 
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>&) {
diff --git a/src/mbgl/util/texture_pool.cpp b/src/mbgl/util/texture_pool.cpp
index 33bca01c05..845796f5df 100644
--- a/src/mbgl/util/texture_pool.cpp
+++ b/src/mbgl/util/texture_pool.cpp
@@ -1,4 +1,5 @@
 #include <mbgl/util/texture_pool.hpp>
+#include <mbgl/map/environment.hpp>
 
 #include <vector>
 
@@ -42,17 +43,9 @@ void TexturePool::removeTextureID(GLuint texture_id) {
 }
 
 void TexturePool::clearTextureIDs() {
-    std::vector<GLuint> ids_to_remove;
-    ids_to_remove.reserve(texture_ids.size());
-
-    for (std::set<GLuint>::iterator id_iterator = texture_ids.begin();
-         id_iterator != texture_ids.end(); ++id_iterator) {
-        ids_to_remove.push_back(*id_iterator);
-    }
-
-    if (!ids_to_remove.empty()) {
-        MBGL_CHECK_ERROR(glDeleteTextures((GLsizei)ids_to_remove.size(), &ids_to_remove[0]));
+    auto& env = Environment::Get();
+    for (auto texture : texture_ids) {
+        env.abandonTexture(texture);
     }
-
     texture_ids.clear();
 }