Replace rather than update Metal buffers (#1842)

include/mbgl/gfx/rendering_stats.hpp

@@ -10,16 +10,36 @@ struct RenderingStats {
     RenderingStats() = default;
     bool isZero() const;
 
+    /// Number of frames rendered
     int numFrames = 0;
+    /// Number of draw calls (`glDrawElements`, `drawIndexedPrimitives`, etc.) executed during the most recent frame
     int numDrawCalls = 0;
+    /// Total number of draw calls executed during all the frames
     int totalDrawCalls = 0;
 
+    /// Total number of textures created
     int numCreatedTextures = 0;
+    /// Net textures
     int numActiveTextures = 0;
+    /// Net texture bindings
     int numTextureBindings = 0;
+    /// Number of times a texture was updated
     int numTextureUpdates = 0;
+    /// Number of bytes used in texture updates
     std::size_t textureUpdateBytes = 0;
 
+    /// Number of buffers created
+    std::size_t totalBuffers = 0;
+    /// Number of SDK-specific buffers created
+    std::size_t totalBufferObjs = 0;
+    /// Number of times a buffer is updated
+    std::size_t bufferUpdates = 0;
+    /// Number of times an SDK-specific buffer is updated
+    std::size_t bufferObjUpdates = 0;
+    /// Sum of update sizes
+    std::size_t bufferUpdateBytes = 0;
+
+    /// Number of active buffers
     int numBuffers = 0;
     int numFrameBuffers = 0;
 

include/mbgl/mtl/context.hpp

@@ -66,9 +66,7 @@ class Context final : public gfx::Context {
     MTLTexturePtr createMetalTexture(MTLTextureDescriptorPtr textureDescriptor) const;
     MTLSamplerStatePtr createMetalSamplerState(MTLSamplerDescriptorPtr samplerDescriptor) const;
 
-    void clear();
-
-    // Actually remove the objects we marked as abandoned with the above methods.
+    /// Called at the end of a frame.
     void performCleanup() override;
 
     void reduceMemoryUsage() override {}

platform/ios/benchmark/MBXBenchViewController.mm

@@ -260,6 +260,14 @@ - (void)startBenchmarkIteration
         // NSLog(@"Total uploads with invalid segs: %zu", mbgl::uploadInvalidSegments);
         // NSLog(@"Total uploads with build: %zu", mbgl::uploadBuildCount);
 
+#if !defined(NDEBUG)
+        // Clean up and show rendering stats, as in `destroyCoreObjects` from tests.
+        // TODO: This doesn't clean up everything, what are we missing?
+        map.reset();
+        observer.reset();
+        frontend.reset();
+#endif // !defined(NDEBUG)
+
         // this does not shut the application down correctly,
         // and results in an assertion failure in thread-local code
         //exit(0);

src/mbgl/gfx/context.hpp

@@ -65,17 +65,17 @@ class Context {
     virtual ~Context() = default;
 
 public:
-    // Called at the end of a frame.
+    /// Called at the end of a frame.
     virtual void performCleanup() = 0;
 
-    // Called when the app receives a memory warning and before it goes to the background.
+    /// Called when the app receives a memory warning and before it goes to the background.
     virtual void reduceMemoryUsage() = 0;
 
 public:
     virtual std::unique_ptr<OffscreenTexture> createOffscreenTexture(Size, TextureChannelDataType) = 0;
 
 public:
-    // Creates an empty texture with the specified dimensions.
+    /// Creates an empty texture with the specified dimensions.
     Texture createTexture(const Size size,
                           TexturePixelType format = TexturePixelType::RGBA,
                           TextureChannelDataType type = TextureChannelDataType::UnsignedByte) {

src/mbgl/gfx/rendering_stats.cpp

@@ -32,6 +32,11 @@ RenderingStats& RenderingStats::operator+=(const RenderingStats& r) {
     numTextureBindings += r.numTextureBindings;
     numTextureUpdates += r.numTextureUpdates;
     textureUpdateBytes += r.textureUpdateBytes;
+    totalBuffers += r.totalBuffers;
+    totalBufferObjs += r.totalBufferObjs;
+    bufferUpdates += r.bufferUpdates;
+    bufferObjUpdates += r.bufferObjUpdates;
+    bufferUpdateBytes += r.bufferUpdateBytes;
     numBuffers += r.numBuffers;
     numFrameBuffers += r.numFrameBuffers;
     numIndexBuffers += r.numIndexBuffers;
@@ -58,14 +63,17 @@ std::string RenderingStats::toString(std::string_view sep) const {
        << "totalDrawCalls = " << totalDrawCalls << sep << "numCreatedTextures = " << numCreatedTextures << sep
        << "numActiveTextures = " << numActiveTextures << sep << "numTextureBindings = " << numTextureBindings << sep
        << "numTextureUpdates = " << numTextureUpdates << sep << "textureUpdateBytes = " << textureUpdateBytes << sep
-       << "numBuffers = " << numBuffers << sep << "numFrameBuffers = " << numFrameBuffers << sep
-       << "numIndexBuffers = " << numIndexBuffers << sep << "indexUpdateBytes = " << indexUpdateBytes << sep
-       << "numVertexBuffers = " << numVertexBuffers << sep << "vertexUpdateBytes = " << vertexUpdateBytes << sep
-       << "numUniformBuffers = " << numUniformBuffers << sep << "numUniformUpdates = " << numUniformUpdates << sep
-       << "uniformUpdateBytes = " << uniformUpdateBytes << sep << "memTextures = " << memTextures << sep
-       << "memBuffers = " << memBuffers << sep << "memIndexBuffers = " << memIndexBuffers << sep
-       << "memVertexBuffers = " << memVertexBuffers << sep << "memUniformBuffers = " << memUniformBuffers << sep
-       << "stencilClears = " << stencilClears << sep << "stencilUpdates = " << stencilUpdates << sep;
+       << "totalBuffers = " << totalBuffers << sep << "totalBufferObjs = " << totalBufferObjs << sep
+       << "bufferUpdates = " << bufferUpdates << sep << "bufferObjUpdates = " << bufferObjUpdates << sep
+       << "bufferUpdateBytes = " << bufferUpdateBytes << sep << "numBuffers = " << numBuffers << sep
+       << "numFrameBuffers = " << numFrameBuffers << sep << "numIndexBuffers = " << numIndexBuffers << sep
+       << "indexUpdateBytes = " << indexUpdateBytes << sep << "numVertexBuffers = " << numVertexBuffers << sep
+       << "vertexUpdateBytes = " << vertexUpdateBytes << sep << "numUniformBuffers = " << numUniformBuffers << sep
+       << "numUniformUpdates = " << numUniformUpdates << sep << "uniformUpdateBytes = " << uniformUpdateBytes << sep
+       << "memTextures = " << memTextures << sep << "memBuffers = " << memBuffers << sep
+       << "memIndexBuffers = " << memIndexBuffers << sep << "memVertexBuffers = " << memVertexBuffers << sep
+       << "memUniformBuffers = " << memUniformBuffers << sep << "stencilClears = " << stencilClears << sep
+       << "stencilUpdates = " << stencilUpdates << sep;
     return ss.str();
 }
 #endif

src/mbgl/gl/context.cpp

@@ -78,6 +78,9 @@ Context::Context(RendererBackend& backend_)
 Context::~Context() noexcept {
     if (cleanupOnDestruction) {
         reset();
+#if !defined(NDEBUG)
+        Log::Debug(Event::General, "Rendering Stats:\n" + stats.toString("\n"));
+#endif
         assert(stats.isZero());
     }
 }

src/mbgl/mtl/buffer_resource.cpp

@@ -45,8 +45,13 @@ BufferResource::BufferResource(Context& context_,
     }
 
     if (isValid()) {
-        context.renderingStats().numBuffers++;
-        context.renderingStats().memBuffers += size;
+        auto& stats = context.renderingStats();
+        stats.numBuffers++;
+        stats.memBuffers += size;
+        stats.totalBuffers++;
+        if (buffer) {
+            stats.totalBufferObjs++;
+        }
     }
 }
 
@@ -85,20 +90,58 @@ BufferResource& BufferResource::operator=(BufferResource&& other) noexcept {
     return *this;
 }
 
-void BufferResource::update(const void* data, std::size_t updateSize, std::size_t offset) noexcept {
+void BufferResource::update(const void* newData, std::size_t updateSize, std::size_t offset) noexcept {
     assert(size >= 0 && updateSize + offset <= size);
     updateSize = std::min(updateSize, size - offset);
+    if (updateSize <= 0) {
+        return;
+    }
 
-    if (updateSize > 0) {
-        if (buffer) {
-            if (auto* content = static_cast<uint8_t*>(buffer->contents())) {
-                std::memcpy(content + offset, data, updateSize);
-            }
+    auto& stats = context.renderingStats();
+    if (buffer) {
+        // Until we can be sure that the buffer is not still in use to render the
+        // previous frame, replace it with a new buffer instead of updating it.
+
+        auto& device = context.getBackend().getDevice();
+        const uint8_t* newBufferSource = nullptr;
+        std::unique_ptr<uint8_t[]> tempBuffer;
+
+        // `[MTLBuffer contents]` may involve memory mapping and/or synchronization.  If the entire
+        // buffer is being replaced, avoid accessing the old one by creating the new buffer directly
+        // from the given data. If it's just being updated, apply the update to a local buffer to
+        // avoid needing to access the new buffer.
+        const bool updateIsEntireBuffer = (offset == 0 && updateSize == size);
+        if (updateIsEntireBuffer) {
+            newBufferSource = static_cast<const uint8_t*>(newData);
         } else {
-            std::memcpy(raw.data() + offset, data, updateSize);
+            if (auto* const oldContent = static_cast<uint8_t*>(buffer->contents())) {
+                tempBuffer.reset(new (std::nothrow) uint8_t[size]);
+                assert(tempBuffer);
+                if (tempBuffer) {
+                    memcpy(tempBuffer.get(), oldContent, size);
+                    memcpy(tempBuffer.get() + offset, newData, updateSize);
+                    newBufferSource = tempBuffer.get();
+                }
+            }
+        }
+
+        if (newBufferSource) {
+            auto newBuffer = NS::TransferPtr(device->newBuffer(newBufferSource, size, usage));
+            assert(newBuffer);
+            if (newBuffer) {
+                buffer = std::move(newBuffer);
+                stats.totalBuffers++;
+                stats.totalBufferObjs++;
+                stats.bufferObjUpdates++;
+                stats.bufferUpdateBytes += updateSize;
+            }
         }
-        version++;
+    } else {
+        std::memcpy(raw.data() + offset, newData, updateSize);
+        stats.bufferUpdateBytes += updateSize;
     }
+    stats.bufferUpdates++;
+    version++;
 }
 
 bool BufferResource::needReBind(VersionType version_) const noexcept {

src/mbgl/mtl/context.cpp

@@ -158,12 +158,9 @@ MTLSamplerStatePtr Context::createMetalSamplerState(MTLSamplerDescriptorPtr samp
     return NS::TransferPtr(backend.getDevice()->newSamplerState(samplerDescriptor.get()));
 }
 
-void Context::clear() {
+void Context::performCleanup() {
     stats.numDrawCalls = 0;
     stats.numFrames++;
-}
-
-void Context::performCleanup() {
     clipMaskUniformsBufferUsed = false;
 }
 

src/mbgl/mtl/drawable.cpp

@@ -501,6 +501,8 @@ void Drawable::upload(gfx::UploadPass& uploadPass_) {
     }
 
     if (impl->indexes->getDirty()) {
+        // Create or update a buffer for the index data.  We don't update any
+        // existing buffer because it may still be in use by the previous frame.
         auto indexBufferResource{uploadPass.createIndexBufferResource(
             impl->indexes->data(), impl->indexes->bytes(), usage, /*persistent=*/false)};
         auto indexBuffer = std::make_unique<gfx::IndexBuffer>(impl->indexes->elements(),

src/mbgl/mtl/render_pass.cpp

@@ -48,7 +48,7 @@ RenderPass::RenderPass(CommandEncoder& commandEncoder_, const char* name, const
     // Let the encoder pass along any groups pushed to it after this
     commandEncoder.trackRenderPass(this);
 
-    commandEncoder.context.clear();
+    commandEncoder.context.performCleanup();
 }
 
 RenderPass::~RenderPass() {

test/map/map.test.cpp

@@ -1606,10 +1606,6 @@ TEST(Map, ObserveShaderRegistration) {
 TEST(Map, StencilOverflow) {
     MapTest<> test;
 
-    const auto& backend = test.frontend.getBackend();
-    gfx::BackendScope scope{*backend};
-    const auto& context = backend->getContext();
-
     auto& style = test.map.getStyle();
     style.loadJSON("{}");
 
@@ -1627,17 +1623,17 @@ TEST(Map, StencilOverflow) {
     style.addLayer(std::make_unique<style::FillLayer>("fill", "custom"));
 
     test.map.jumpTo(CameraOptions().withZoom(5));
-    test.frontend.render(test.map);
+    auto result = test.frontend.render(test.map);
 
     // In drawable builds, no drawables are built because no bucket/tiledata is available.
 #if MLN_DRAWABLE_RENDERER
-    ASSERT_LE(0, context.renderingStats().stencilUpdates);
+    ASSERT_LE(0, result.stats.stencilUpdates);
 #else
-    ASSERT_LT(0, context.renderingStats().stencilClears);
+    ASSERT_LT(0, result.stats.stencilClears);
 #endif // MLN_DRAWABLE_RENDERER
 
 #if !defined(NDEBUG)
-    Log::Info(Event::General, context.renderingStats().toString("\n"));
+    Log::Info(Event::General, result.stats.toString("\n"));
 #endif // !defined(NDEBUG)
 
     // TODO: confirm that the stencil masking actually worked