From b41e6dfd5c06de0a85294c154e15c409ce84e901 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 30 Sep 2025 16:06:58 -0700 Subject: [PATCH] fix several issues CircularQueue (#9271) - pop_back() didn't call the object's dtor - added a dtor so objects can't be leaked - made it moveable but not copiable - added unit tests --- filament/src/FrameInfo.h | 65 +++++++- filament/test/CMakeLists.txt | 1 + filament/test/test_CircularQueue.cpp | 230 +++++++++++++++++++++++++++ 3 files changed, 291 insertions(+), 5 deletions(-) create mode 100644 filament/test/test_CircularQueue.cpp diff --git a/filament/src/FrameInfo.h b/filament/src/FrameInfo.h index 668d8a23fe..5393f789fc 100644 --- a/filament/src/FrameInfo.h +++ b/filament/src/FrameInfo.h @@ -51,14 +51,36 @@ struct FrameInfo { struct FrameInfoImpl : public details::FrameInfo { using clock = std::chrono::steady_clock; using time_point = clock::time_point; - uint32_t const frameId; + uint32_t frameId; time_point beginFrame; // main thread beginFrame time time_point endFrame; // main thread endFrame time time_point backendBeginFrame; // backend thread beginFrame time (makeCurrent time) time_point backendEndFrame; // backend thread endFrame time (present time) std::atomic_bool ready{}; // true once backend thread has populated its data - explicit FrameInfoImpl(uint32_t const frameId) noexcept - : frameId(frameId) { + explicit FrameInfoImpl(uint32_t const id) noexcept + : frameId(id) { + } + + FrameInfoImpl(FrameInfoImpl&& rhs) noexcept : + details::FrameInfo(rhs), + frameId(rhs.frameId), + beginFrame(rhs.beginFrame), + endFrame(rhs.endFrame), + backendBeginFrame(rhs.backendBeginFrame), + backendEndFrame(rhs.backendEndFrame), + ready(rhs.ready.load()) + { + } + + FrameInfoImpl& operator=(FrameInfoImpl&& rhs) noexcept { + details::FrameInfo::operator=(rhs); + frameId = rhs.frameId; + beginFrame = rhs.beginFrame; + endFrame = rhs.endFrame; + backendBeginFrame = rhs.backendBeginFrame; + backendEndFrame = rhs.backendEndFrame; + ready.store(rhs.ready.load()); + return *this; } }; @@ -69,6 +91,38 @@ public: using reference = value_type&; using const_reference = value_type const&; + CircularQueue() = default; + + ~CircularQueue() { + if constexpr (!std::is_trivially_destructible_v) { + for (size_t i = 0, c = mSize; i < c; ++i) { + size_t const index = (mFront + CAPACITY - i) % CAPACITY; + std::destroy_at(std::launder(reinterpret_cast(&mStorage[index]))); + } + } + } + + CircularQueue(const CircularQueue&) = delete; + CircularQueue& operator=(const CircularQueue&) = delete; + + CircularQueue(CircularQueue&& other) noexcept { + for (size_t i = 0; i < other.mSize; i++) { + size_t const index = (other.mFront + CAPACITY - i) % CAPACITY; + new(&mStorage[index]) T(std::move(*std::launder(reinterpret_cast(&other.mStorage[index])))); + } + mFront = other.mFront; + mSize = other.mSize; + other.mSize = 0; + } + + CircularQueue& operator=(CircularQueue&& other) noexcept { + if (this != &other) { + this->~CircularQueue(); + new(this) CircularQueue(std::move(other)); + } + return *this; + } + size_t capacity() const { return CAPACITY; } @@ -84,7 +138,8 @@ public: void pop_back() noexcept { assert_invariant(!empty()); --mSize; - std::destroy_at(&mStorage[(mFront - mSize) % CAPACITY]); + size_t const index = (mFront + CAPACITY - mSize) % CAPACITY; + std::destroy_at(std::launder(reinterpret_cast(&mStorage[index]))); } void push_front(T const& v) noexcept { @@ -133,7 +188,7 @@ public: private: using Storage = std::aligned_storage_t; Storage mStorage[CAPACITY]; - uint32_t mFront = 0; // always index 0 + uint32_t mFront = 0; uint32_t mSize = 0; [[nodiscard]] inline uint32_t advance(uint32_t const v) noexcept { return (v + 1) % CAPACITY; diff --git a/filament/test/CMakeLists.txt b/filament/test/CMakeLists.txt index 59d840d811..717d9ed5c1 100644 --- a/filament/test/CMakeLists.txt +++ b/filament/test/CMakeLists.txt @@ -43,6 +43,7 @@ list(APPEND RESGEN_SOURCE ${DUMMY_SRC}) if (TNT_DEV) add_executable(test_${TARGET} filament_AtlasAllocator_test.cpp + test_CircularQueue.cpp filament_test_exposure.cpp filament_rendering_test.cpp filament_bimap_test.cpp diff --git a/filament/test/test_CircularQueue.cpp b/filament/test/test_CircularQueue.cpp new file mode 100644 index 0000000000..15041da894 --- /dev/null +++ b/filament/test/test_CircularQueue.cpp @@ -0,0 +1,230 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include "../src/FrameInfo.h" + +#include +#include + +using namespace filament; + +TEST(CircularQueue, Empty) { + CircularQueue const q{}; + EXPECT_EQ(q.capacity(), 4); + EXPECT_EQ(q.size(), 0); + EXPECT_TRUE(q.empty()); +} + +TEST(CircularQueue, PushFront) { + CircularQueue q; + q.push_front(1); + EXPECT_EQ(q.size(), 1); + EXPECT_FALSE(q.empty()); + EXPECT_EQ(q.front(), 1); + + q.push_front(2); + EXPECT_EQ(q.size(), 2); + EXPECT_EQ(q.front(), 2); + EXPECT_EQ(q[1], 1); + + q.push_front(3); + q.push_front(4); + EXPECT_EQ(q.size(), 4); + EXPECT_EQ(q.front(), 4); + EXPECT_EQ(q[1], 3); + EXPECT_EQ(q[2], 2); + EXPECT_EQ(q[3], 1); +} + +TEST(CircularQueue, EmplaceFront) { + CircularQueue, 2> q; + q.emplace_front(1, 2); + EXPECT_EQ(q.size(), 1); + EXPECT_EQ(q.front(), std::make_pair(1, 2)); + + q.emplace_front(3, 4); + EXPECT_EQ(q.size(), 2); + EXPECT_EQ(q.front(), std::make_pair(3, 4)); + EXPECT_EQ(q[1], std::make_pair(1, 2)); +} + + +TEST(CircularQueue, PopBack) { + CircularQueue q; + q.push_front(1); + q.push_front(2); + q.push_front(3); + EXPECT_EQ(q.size(), 3); + + q.pop_back(); + EXPECT_EQ(q.size(), 2); + EXPECT_EQ(q.front(), 3); + EXPECT_EQ(q[1], 2); + + q.pop_back(); + EXPECT_EQ(q.size(), 1); + EXPECT_EQ(q.front(), 3); + + q.pop_back(); + EXPECT_EQ(q.size(), 0); + EXPECT_TRUE(q.empty()); +} + +TEST(CircularQueue, PushPop) { + CircularQueue q; + q.push_front(1); + q.push_front(2); + q.push_front(3); + EXPECT_EQ(q.size(), 3); + + q.pop_back(); + EXPECT_EQ(q.size(), 2); + EXPECT_EQ(q.front(), 3); + EXPECT_EQ(q[1], 2); + + q.push_front(4); + EXPECT_EQ(q.size(), 3); + EXPECT_EQ(q.front(), 4); + EXPECT_EQ(q[1], 3); + EXPECT_EQ(q[2], 2); + + q.pop_back(); + q.pop_back(); + EXPECT_EQ(q.size(), 1); + EXPECT_EQ(q.front(), 4); +} + +struct TestObject { + static int construction_count; + static int destruction_count; + TestObject() { construction_count++; } + TestObject(const TestObject&) { construction_count++; } + TestObject(TestObject&&) noexcept { construction_count++; } + ~TestObject() { destruction_count++; } +}; + +int TestObject::construction_count = 0; +int TestObject::destruction_count = 0; + + +TEST(CircularQueue, ObjectLifecycle) { + TestObject::construction_count = 0; + TestObject::destruction_count = 0; + { + CircularQueue q; + EXPECT_EQ(TestObject::construction_count, 0); + EXPECT_EQ(TestObject::destruction_count, 0); + + q.emplace_front(); + EXPECT_EQ(TestObject::construction_count, 1); + EXPECT_EQ(TestObject::destruction_count, 0); + + TestObject const t; + EXPECT_EQ(TestObject::construction_count, 2); + q.push_front(t); + EXPECT_EQ(TestObject::construction_count, 3); + EXPECT_EQ(TestObject::destruction_count, 0); + + q.push_front(TestObject{}); + EXPECT_EQ(TestObject::construction_count, 5); // 4 for {}, 5 for move + EXPECT_EQ(TestObject::destruction_count, 1); // destruction of temporary + + EXPECT_EQ(q.size(), 3); + + q.pop_back(); + EXPECT_EQ(TestObject::destruction_count, 2); + + q.pop_back(); + EXPECT_EQ(TestObject::destruction_count, 3); + } + EXPECT_EQ(TestObject::construction_count, 5); + EXPECT_EQ(TestObject::destruction_count, 5); +} + +struct PopBackTestObject { + int id; + static std::vector destroyed_ids; + + PopBackTestObject(int const id) : id(id) {} + ~PopBackTestObject() { + destroyed_ids.push_back(id); + } +}; +std::vector PopBackTestObject::destroyed_ids; + +TEST(CircularQueue, PopBackDestroysCorrectObject) { + PopBackTestObject::destroyed_ids.clear(); + { + CircularQueue q; + q.emplace_front(1); + q.emplace_front(2); + q.emplace_front(3); + + // The queue contains [3, 2, 1], where 3 is at the front and 1 is at the back. + q.pop_back(); // Should destroy 1. + ASSERT_EQ(PopBackTestObject::destroyed_ids.size(), 1); + EXPECT_EQ(PopBackTestObject::destroyed_ids[0], 1); + + q.pop_back(); // Should destroy 2. + ASSERT_EQ(PopBackTestObject::destroyed_ids.size(), 2); + EXPECT_EQ(PopBackTestObject::destroyed_ids[1], 2); + + q.emplace_front(4); + // The queue contains [4, 3]. 3 is at the back. + q.pop_back(); // Should destroy 3. + ASSERT_EQ(PopBackTestObject::destroyed_ids.size(), 3); + EXPECT_EQ(PopBackTestObject::destroyed_ids[2], 3); + } + // Queue is destroyed, remaining object with id 4 is destroyed. + ASSERT_EQ(PopBackTestObject::destroyed_ids.size(), 4); + EXPECT_EQ(PopBackTestObject::destroyed_ids[3], 4); +} + +TEST(CircularQueue, PointerStability) { + CircularQueue q; + q.push_front(1); + q.push_front(2); + + // q is [2, 1] + const int* p1 = &q[1]; + EXPECT_EQ(*p1, 1); + + q.push_front(3); + q.push_front(4); + // q is [4, 3, 2, 1] + EXPECT_EQ(*p1, 1); + + const int* p3 = &q[1]; + EXPECT_EQ(*p3, 3); + + q.pop_back(); // removes 1 + // q is [4, 3, 2] + // p1 is now invalid, but p3 should be fine. + EXPECT_EQ(*p3, 3); + + q.push_front(5); + q.push_front(6); + // q is [6, 5, 4, 3, 2] + // p3 is still fine. + EXPECT_EQ(*p3, 3); + + q.pop_back(); // removes 2 + q.pop_back(); // removes 3 + // q is [6, 5, 4] + // p3 is now invalid. +}