From ffcf132f20032aeb480b627d386ef5954884ef34 Mon Sep 17 00:00:00 2001 From: Tim Sylvester Date: Mon, 20 Nov 2023 12:58:19 -0800 Subject: [PATCH] Replace rather than update Metal buffers (#1842) --- include/mbgl/gfx/rendering_stats.hpp | 20 ++++++ include/mbgl/mtl/context.hpp | 4 +- .../ios/benchmark/MBXBenchViewController.mm | 8 +++ src/mbgl/gfx/context.hpp | 6 +- src/mbgl/gfx/rendering_stats.cpp | 24 ++++--- src/mbgl/gl/context.cpp | 3 + src/mbgl/mtl/buffer_resource.cpp | 63 ++++++++++++++++--- src/mbgl/mtl/context.cpp | 5 +- src/mbgl/mtl/drawable.cpp | 2 + src/mbgl/mtl/render_pass.cpp | 2 +- test/map/map.test.cpp | 12 ++-- 11 files changed, 112 insertions(+), 37 deletions(-) diff --git a/include/mbgl/gfx/rendering_stats.hpp b/include/mbgl/gfx/rendering_stats.hpp index b64804c05e8..22918e28ae0 100644 --- a/include/mbgl/gfx/rendering_stats.hpp +++ b/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; diff --git a/include/mbgl/mtl/context.hpp b/include/mbgl/mtl/context.hpp index 4be718e8ae1..7063c72b03d 100644 --- a/include/mbgl/mtl/context.hpp +++ b/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 {} diff --git a/platform/ios/benchmark/MBXBenchViewController.mm b/platform/ios/benchmark/MBXBenchViewController.mm index b01be05c24f..12b167ea369 100644 --- a/platform/ios/benchmark/MBXBenchViewController.mm +++ b/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); diff --git a/src/mbgl/gfx/context.hpp b/src/mbgl/gfx/context.hpp index ce43043790b..8029f4f2685 100644 --- a/src/mbgl/gfx/context.hpp +++ b/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 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) { diff --git a/src/mbgl/gfx/rendering_stats.cpp b/src/mbgl/gfx/rendering_stats.cpp index 39794fb1589..a0097bed65c 100644 --- a/src/mbgl/gfx/rendering_stats.cpp +++ b/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 diff --git a/src/mbgl/gl/context.cpp b/src/mbgl/gl/context.cpp index d8c2aa03860..842a1eb5874 100644 --- a/src/mbgl/gl/context.cpp +++ b/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()); } } diff --git a/src/mbgl/mtl/buffer_resource.cpp b/src/mbgl/mtl/buffer_resource.cpp index c6ed8d87c40..3ed03798005 100644 --- a/src/mbgl/mtl/buffer_resource.cpp +++ b/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(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 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(newData); } else { - std::memcpy(raw.data() + offset, data, updateSize); + if (auto* const oldContent = static_cast(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 { diff --git a/src/mbgl/mtl/context.cpp b/src/mbgl/mtl/context.cpp index 4e4167af8b7..699ea7c095e 100644 --- a/src/mbgl/mtl/context.cpp +++ b/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; } diff --git a/src/mbgl/mtl/drawable.cpp b/src/mbgl/mtl/drawable.cpp index 4453823fc50..1bb868dc246 100644 --- a/src/mbgl/mtl/drawable.cpp +++ b/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(impl->indexes->elements(), diff --git a/src/mbgl/mtl/render_pass.cpp b/src/mbgl/mtl/render_pass.cpp index a8f7b731104..6a06762f75f 100644 --- a/src/mbgl/mtl/render_pass.cpp +++ b/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() { diff --git a/test/map/map.test.cpp b/test/map/map.test.cpp index c9116651381..9b0827164cb 100644 --- a/test/map/map.test.cpp +++ b/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("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