From 3debb26a9e2dd278af9465d939a650bdb3eb9f2e Mon Sep 17 00:00:00 2001 From: Bryan Bernhart Date: Tue, 29 Oct 2019 22:49:43 +0000 Subject: [PATCH] Limit heap growth when uploading with SetSubData. Removes unbounded heap growth by capping the heap size to 4MB and falling back to direct allocation for larger requests. BUG=dawn:239 Change-Id: I9ae660809e3c0c539fbcfbee4afcf6fb1f466355 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/12720 Reviewed-by: Austin Eng Commit-Queue: Bryan Bernhart --- src/dawn_native/DynamicUploader.cpp | 30 ++++++++++++++--------- src/dawn_native/DynamicUploader.h | 5 ++-- src/tests/perf_tests/BufferUploadPerf.cpp | 4 --- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/dawn_native/DynamicUploader.cpp b/src/dawn_native/DynamicUploader.cpp index 9996ee1edf..be92ea0959 100644 --- a/src/dawn_native/DynamicUploader.cpp +++ b/src/dawn_native/DynamicUploader.cpp @@ -18,9 +18,9 @@ namespace dawn_native { - DynamicUploader::DynamicUploader(DeviceBase* device, uint64_t size) : mDevice(device) { - mRingBuffers.emplace_back( - std::unique_ptr(new RingBuffer{nullptr, RingBufferAllocator(size)})); + DynamicUploader::DynamicUploader(DeviceBase* device) : mDevice(device) { + mRingBuffers.emplace_back(std::unique_ptr( + new RingBuffer{nullptr, RingBufferAllocator(kRingBufferSize)})); } void DynamicUploader::ReleaseStagingBuffer(std::unique_ptr stagingBuffer) { @@ -29,6 +29,19 @@ namespace dawn_native { } ResultOrError DynamicUploader::Allocate(uint64_t allocationSize, Serial serial) { + // Disable further sub-allocation should the request be too large. + if (allocationSize > kRingBufferSize) { + std::unique_ptr stagingBuffer; + DAWN_TRY_ASSIGN(stagingBuffer, mDevice->CreateStagingBuffer(allocationSize)); + + UploadHandle uploadHandle; + uploadHandle.mappedBuffer = static_cast(stagingBuffer->GetMappedPointer()); + uploadHandle.stagingBuffer = stagingBuffer.get(); + + ReleaseStagingBuffer(std::move(stagingBuffer)); + return uploadHandle; + } + // Note: Validation ensures size is already aligned. // First-fit: find next smallest buffer large enough to satisfy the allocation request. RingBuffer* targetRingBuffer = mRingBuffers.back().get(); @@ -49,18 +62,11 @@ namespace dawn_native { startOffset = targetRingBuffer->mAllocator.Allocate(allocationSize, serial); } - // Upon failure, append a newly created (and much larger) ring buffer to fulfill the + // Upon failure, append a newly created ring buffer to fulfill the // request. if (startOffset == RingBufferAllocator::kInvalidOffset) { - // Compute the new max size (in powers of two to preserve alignment). - size_t newMaxSize = targetRingBuffer->mAllocator.GetSize() * 2; - while (newMaxSize < allocationSize) { - newMaxSize *= 2; - } - - // TODO(bryan.bernhart@intel.com): Fall-back to no sub-allocations should this fail. mRingBuffers.emplace_back(std::unique_ptr( - new RingBuffer{nullptr, RingBufferAllocator(newMaxSize)})); + new RingBuffer{nullptr, RingBufferAllocator(kRingBufferSize)})); targetRingBuffer = mRingBuffers.back().get(); startOffset = targetRingBuffer->mAllocator.Allocate(allocationSize, serial); diff --git a/src/dawn_native/DynamicUploader.h b/src/dawn_native/DynamicUploader.h index 068eebd060..8210b035b2 100644 --- a/src/dawn_native/DynamicUploader.h +++ b/src/dawn_native/DynamicUploader.h @@ -31,7 +31,7 @@ namespace dawn_native { class DynamicUploader { public: - DynamicUploader(DeviceBase* device, uint64_t size = kBaseUploadBufferSize); + DynamicUploader(DeviceBase* device); ~DynamicUploader() = default; // We add functions to Release StagingBuffers to the DynamicUploader as there's @@ -44,8 +44,7 @@ namespace dawn_native { void Deallocate(Serial lastCompletedSerial); private: - // TODO(bryan.bernhart@intel.com): Figure out this value. - static constexpr uint64_t kBaseUploadBufferSize = 64000; + static constexpr uint64_t kRingBufferSize = 4 * 1024 * 1024; struct RingBuffer { std::unique_ptr mStagingBuffer; diff --git a/src/tests/perf_tests/BufferUploadPerf.cpp b/src/tests/perf_tests/BufferUploadPerf.cpp index d9a6ca0d07..181547d28d 100644 --- a/src/tests/perf_tests/BufferUploadPerf.cpp +++ b/src/tests/perf_tests/BufferUploadPerf.cpp @@ -142,10 +142,6 @@ void BufferUploadPerf::Step() { } TEST_P(BufferUploadPerf, Run) { - // TODO(crbug.com/dawn/239): Investigate why large buffer uploads via SetSubData fail. - DAWN_SKIP_TEST_IF(GetParam().uploadMethod == UploadMethod::SetSubData && - GetParam().uploadSize == UploadSize::BufferSize_16MB); - RunTest(); }