Scarfski/filament
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix several issues CircularQueue (#9271)

Browse Source 
- 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
This commit is contained in:
Mathias Agopian
2025-09-30 16:06:58 -07:00
committed by GitHub
parent 8413c84284
commit b41e6dfd5c
3 changed files with 291 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
filament/src/FrameInfo.h
View File

@@ -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<T>) {
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<T*>(&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<T*>(&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<T*>(&mStorage[index])));
}
void push_front(T const& v) noexcept {
@@ -133,7 +188,7 @@ public:
private:
using Storage = std::aligned_storage_t<sizeof(T), alignof(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;

1
filament/test/CMakeLists.txt
View File

@@ -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

230
filament/test/test_CircularQueue.cpp Normal file
View File

@@ -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 <gtest/gtest.h>
#include "../src/FrameInfo.h"
#include <utility>
#include <vector>
using namespace filament;
TEST(CircularQueue, Empty) {
CircularQueue<int, 4> const q{};
EXPECT_EQ(q.capacity(), 4);
EXPECT_EQ(q.size(), 0);
EXPECT_TRUE(q.empty());
}
TEST(CircularQueue, PushFront) {
CircularQueue<int, 4> 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<std::pair<int, int>, 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<int, 4> 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<int, 3> 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<TestObject, 3> 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<int> destroyed_ids;
PopBackTestObject(int const id) : id(id) {}
~PopBackTestObject() {
destroyed_ids.push_back(id);
}
};
std::vector<int> PopBackTestObject::destroyed_ids;
TEST(CircularQueue, PopBackDestroysCorrectObject) {
PopBackTestObject::destroyed_ids.clear();
{
CircularQueue<PopBackTestObject, 4> 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<int, 5> 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.
}