From 7fe1ee3fd5e9ef0fb869f2f056e24dfb21c5dfc7 Mon Sep 17 00:00:00 2001 From: Eliza <4576666+elizagamedev@users.noreply.github.com> Date: Fri, 3 Oct 2025 11:21:02 -0700 Subject: [PATCH] utils: RefCountedInternPool/RefCountedMap (#9284) * utils: RefCountedInternPool/RefCountedMap First, introduce RefCountedInternPool, a reference counted intern pool of Slice. Just acquire() a slice that you want and you're guaranteed to get exactly one canonical value-equal Slice back. Additionally, introduce the concept of NullValue to RefCountedMap. A NullValue defines what should be considered an uninitialized value; by default, it's the default value of that type (0 for ints, nullptr for pointers, etc). This allows us to lazily-initialize values in the map. A client can acquire() a bunch of different resources which will be initialized only when get(factory) is called. If a client attempts to get() a value without specifying a factory, and the value is not initialized (i.e. equal to NullValue{}()), RefCountedMap will panic. * utils: add unit tests for ref-counted collections * utils: remove C++20 features, fix memory issue * utils: remove RefCounted from InternPool --- libs/utils/CMakeLists.txt | 2 + libs/utils/include/utils/InternPool.h | 146 +++++++++++++++++++++ libs/utils/include/utils/RefCountedMap.h | 99 ++++++++++----- libs/utils/test/test_InternPool.cpp | 154 +++++++++++++++++++++++ libs/utils/test/test_RefCountedMap.cpp | 36 ++++++ 5 files changed, 406 insertions(+), 31 deletions(-) create mode 100644 libs/utils/include/utils/InternPool.h create mode 100644 libs/utils/test/test_InternPool.cpp diff --git a/libs/utils/CMakeLists.txt b/libs/utils/CMakeLists.txt index eb9f2d0b0f..9b013e2015 100644 --- a/libs/utils/CMakeLists.txt +++ b/libs/utils/CMakeLists.txt @@ -26,6 +26,7 @@ set(DIST_HDRS ${PUBLIC_HDR_DIR}/${TARGET}/EntityManager.h ${PUBLIC_HDR_DIR}/${TARGET}/FixedCapacityVector.h ${PUBLIC_HDR_DIR}/${TARGET}/Hash.h + ${PUBLIC_HDR_DIR}/${TARGET}/InternPool.h ${PUBLIC_HDR_DIR}/${TARGET}/Invocable.h ${PUBLIC_HDR_DIR}/${TARGET}/Log.h ${PUBLIC_HDR_DIR}/${TARGET}/Logger.h @@ -173,6 +174,7 @@ set(TEST_SRCS test/test_FixedCapacityVector.cpp test/test_FixedCircularBuffer.cpp test/test_Hash.cpp + test/test_InternPool.cpp test/test_JobSystem.cpp test/test_QuadTreeArray.cpp test/test_RangeMap.cpp diff --git a/libs/utils/include/utils/InternPool.h b/libs/utils/include/utils/InternPool.h new file mode 100644 index 0000000000..b9ccccbd33 --- /dev/null +++ b/libs/utils/include/utils/InternPool.h @@ -0,0 +1,146 @@ +/* + * Copyright (C) 2025 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. + */ +#ifndef TNT_UTILS_INTERNPOOL_H +#define TNT_UTILS_INTERNPOOL_H + +#include +#include +#include +#include +#include + +#include + +namespace utils { + +/** A reference-counted intern pool of slices of T. */ +template> +class InternPool { + struct HashSlice { + inline size_t operator()(Slice const& slice) const noexcept { + return slice.template hash(); + } + }; + + struct Entry { + uint32_t referenceCount; + FixedCapacityVector value; + }; + + using Map = tsl::robin_map, Entry, HashSlice>; + + static constexpr const char* UTILS_NONNULL MISSING_ENTRY_ERROR_STRING = + "InternPool is missing entry"; + +public: + InternPool() = default; + InternPool(InternPool const& rhs) = delete; + InternPool& operator=(InternPool const& rhs) = delete; + InternPool(InternPool&& rhs) = default; + InternPool& operator=(InternPool&& rhs) = default; + + /** Acquire an interned copy of value. */ + Slice acquire(Slice slice, size_t hash) noexcept { + if (slice.empty()) { + return { nullptr, nullptr }; + } + auto it = mMap.find(slice, hash); + if (it != mMap.end()) { + it.value().referenceCount++; + return it.key(); + } + FixedCapacityVector value(slice); + // TODO: how to use above computed hash here? + return mMap.insert({ value.as_slice(), Entry{ 1, std::move(value) } }).first.key(); + } + + inline Slice acquire(Slice slice) noexcept { + return acquire(slice, HashSlice{}(slice)); + } + + Slice acquire(FixedCapacityVector&& value, size_t hash) noexcept { + if (value.empty()) { + return { nullptr, nullptr }; + } + Slice slice = value.as_slice(); + auto it = mMap.find(slice, hash); + if (it != mMap.end()) { + it.value().referenceCount++; + return it.key(); + } + // TODO: how to use above computed hash here? + return mMap.insert({ slice, Entry{ 1, std::move(value) } }).first.key(); + } + + inline Slice acquire(FixedCapacityVector&& value) noexcept { + size_t hash = HashSlice{}(value.as_slice()); + return acquire(std::move(value), hash); + } + + inline Slice acquire(FixedCapacityVector const& value, size_t hash) noexcept { + return acquire(value.as_slice(), hash); + } + + inline Slice acquire(FixedCapacityVector const& value) noexcept { + Slice slice = value.as_slice(); + return acquire(slice, HashSlice{}(slice)); + } + + /** Release interned value. */ + void release(Slice slice, size_t hash) { + if (slice.empty()) { + return; + } + auto it = mMap.find(slice, hash); + FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; + if (--it.value().referenceCount == 0) { + // TODO: change to erase_fast + mMap.erase(it); + } + } + + inline void release(Slice slice) noexcept { + return release(slice, HashSlice{}(slice)); + } + + inline void release(FixedCapacityVector const& value, size_t hash) noexcept { + return release(value.as_slice(), hash); + } + + inline void release(FixedCapacityVector const& value) noexcept { + Slice slice = value.as_slice(); + return release(slice, HashSlice{}(slice)); + } + + /** Returns true if the pool is empty. */ + inline bool empty() const noexcept { return mMap.empty(); } + + /** Returns hash of value. */ + static size_t hash(Slice slice) noexcept { + return HashSlice{}(slice); + } + + static size_t hash(FixedCapacityVector const& value) noexcept { + return HashSlice{}(value.as_slice()); + } + +private: + Map mMap; +}; + +} // namespace utils + +#endif // TNT_UTILS_INTERNPOOL_H diff --git a/libs/utils/include/utils/RefCountedMap.h b/libs/utils/include/utils/RefCountedMap.h index 4f780f7c84..5fc0b85f5c 100644 --- a/libs/utils/include/utils/RefCountedMap.h +++ b/libs/utils/include/utils/RefCountedMap.h @@ -49,6 +49,13 @@ struct PointerTraits>> { using element_type = typename std::pointer_traits::element_type; }; +template +struct DefaultValue { + T operator()() const noexcept { + return {}; + } +}; + } // namespace refcountedmap /** A reference-counted map. @@ -56,7 +63,8 @@ struct PointerTraits>> { * Don't use RAII here, both because we sometimes want to deliberately leak memory, and because * we're managing GL resources that require more managed destruction. */ -template> +template, + typename NullValue = refcountedmap::DefaultValue> class RefCountedMap { // Use references for the key if the size of the key type is greater than the size of a pointer. using KeyRef = std::conditional_t<(sizeof(Key) > sizeof(void*)), const Key&, Key>; @@ -87,14 +95,14 @@ class RefCountedMap { static constexpr const char* UTILS_NONNULL MISSING_ENTRY_ERROR_STRING = "Cache is missing entry"; + static constexpr const char* UTILS_NONNULL MISSING_VALUE_ERROR_STRING = + "Attempted to get missing value"; public: - /** Acquire a new reference to value by key, initializing it with F if it doesn't exist. + /** Acquire and return a value by key, initializing it with F if it doesn't exist. * - * If T is a pointer type, F returns T, where nullptr indicates a failure to create the object. - * Otherwise, F returns std::optional, where nullopt indicates a failure to create the - * object, and the returned pointer is valid only as long as the next call to acquire() or - * release(). + * If F returns NullValue{}(), this indicates a failure to create the object. If T is a value + * type, the returned pointer is valid only as long as the next call to acquire() or release(). */ template TValue* UTILS_NULLABLE acquire(KeyRef key, size_t hash, F factory) noexcept { @@ -103,21 +111,12 @@ public: it.value().referenceCount++; return &deref(it.value().value); } - if constexpr (refcountedmap::IsPointer) { - T r = factory(); - if (r) { - // TODO: how to use above computed hash here? - return &*mMap.insert({key, Entry{ 1, std::move(r) }}).first.value().value; - } - return nullptr; - } else { - std::optional r = factory(); - if (r) { - // TODO: how to use above computed hash here? - return &mMap.insert({key, Entry{ 1, std::move(r.value()) }}).first.value().value; - } + T r = factory(); + if (r == NullValue{}()) { return nullptr; } + // TODO: how to use above computed hash here? + return &deref(mMap.insert({ key, Entry{ 1, std::move(r) } }).first.value().value); } template @@ -125,18 +124,26 @@ public: return acquire(key, Hash{}(key), std::move(factory)); } - /** Acquire a reference to value by key, panicking if it doesn't exist. + /** Acquire and return a pointer to the value if one exists. * - * This reference is valid only as long as the next call to acquire() or release(). + * It's possible to acquire a key before its value is initialized, in which case this function + * returns nullptr. + * + * If T is a value type, this pointer is valid only as long as the next call to acquire() or + * release(). */ - TValue& acquire(KeyRef key, size_t hash) noexcept { + TValue* UTILS_NULLABLE acquire(KeyRef key, size_t hash) noexcept { auto it = mMap.find(key, hash); - FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; - it.value().referenceCount++; - return deref(it.value().value); + if (it != mMap.end()) { + it.value().referenceCount++; + return &deref(it.value().value); + } + // TODO: how to use above computed hash here? + mMap.insert({ key, Entry{ 1, NullValue{}() } }); + return nullptr; } - inline TValue& acquire(KeyRef key) noexcept { + inline TValue* UTILS_NULLABLE acquire(KeyRef key) noexcept { return acquire(key, Hash{}(key)); } @@ -145,11 +152,13 @@ public: * Panics if no entry found in map. */ template - void release(KeyRef key, size_t hash, F releaser) noexcept { + void release(KeyRef key, size_t hash, F releaser) { auto it = mMap.find(key, hash); FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; if (--it.value().referenceCount == 0) { - releaser(deref(it.value().value)); + if (it.value().value != NullValue{}()){ + releaser(deref(it.value().value)); + } // TODO: change to erase_fast mMap.erase(it); } @@ -164,7 +173,7 @@ public: * * Panics if no entry found in map. */ - void release(KeyRef key, size_t hash) noexcept { + void release(KeyRef key, size_t hash) { auto it = mMap.find(key, hash); FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; if (--it.value().referenceCount == 0) { @@ -177,23 +186,51 @@ public: release(key, Hash{}(key)); } + /** Get a value by key, initializing it with F if it doesn't exist. + * + * If F returns NullValue{}(), this indicates a failure to create the object. If T is a value + * type, the returned pointer is valid only as long as the next call to acquire() or release(). + */ + template + TValue* UTILS_NULLABLE get(KeyRef key, size_t hash, F factory) { + auto it = mMap.find(key, hash); + FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; + const T nullValue = NullValue{}(); + if (it.value().value == nullValue) { + it.value().value = factory(); + if (it.value().value == nullValue) { + return nullptr; + } + } + return &deref(it.value().value); + } + + template + inline TValue* UTILS_NULLABLE get(KeyRef key, F factory) noexcept { + return get(key, Hash{}(key), std::move(factory)); + } + /** Return reference to existing value by key. * * This reference is valid only as long as the next call to acquire() or release(). * * Panics if no entry found in map. */ - TValue& get(KeyRef key, size_t hash) noexcept { + TValue& get(KeyRef key, size_t hash) { auto it = mMap.find(key); FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; + FILAMENT_CHECK_PRECONDITION(it.value().value != NullValue{}()) + << MISSING_VALUE_ERROR_STRING; return deref(it.value().value); } inline TValue& get(KeyRef key) noexcept { return get(key, Hash{}(key)); } - TValue const& get(KeyRef key, size_t hash) const noexcept { + TValue const& get(KeyRef key, size_t hash) const { auto it = mMap.find(key); FILAMENT_CHECK_PRECONDITION(it != mMap.end()) << MISSING_ENTRY_ERROR_STRING; + FILAMENT_CHECK_PRECONDITION(it.value().value != NullValue{}()) + << MISSING_VALUE_ERROR_STRING; return deref(it->second.value); } diff --git a/libs/utils/test/test_InternPool.cpp b/libs/utils/test/test_InternPool.cpp new file mode 100644 index 0000000000..92575bba99 --- /dev/null +++ b/libs/utils/test/test_InternPool.cpp @@ -0,0 +1,154 @@ +/* + * Copyright (C) 2025 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 +#include +#include + +using namespace utils; + +TEST(InternPoolTest, AcquireWithCopy) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + Slice interned = pool.acquire(value); + + EXPECT_FALSE(pool.empty()); + EXPECT_EQ(value.as_slice(), interned); +} + +TEST(InternPoolTest, AcquireWithMove) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + FixedCapacityVector copy = value; + const int* data = value.data(); + Slice interned = pool.acquire(std::move(value)); + + EXPECT_FALSE(pool.empty()); + EXPECT_EQ(copy, interned); + EXPECT_EQ(data, interned.data()); +} + +TEST(InternPoolTest, InternIsUnique) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + Slice interned1 = pool.acquire(value); + Slice interned2 = pool.acquire(value); + Slice interned3 = pool.acquire(value); + + EXPECT_FALSE(pool.empty()); + EXPECT_EQ(interned1.begin(), interned2.begin()); + EXPECT_EQ(interned1.begin(), interned3.begin()); + EXPECT_EQ(interned1.end(), interned2.end()); + EXPECT_EQ(interned1.end(), interned3.end()); +} + +TEST(InternPoolTest, ReleaseByValue) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + Slice interned = pool.acquire(value); + + EXPECT_FALSE(pool.empty()); + + pool.release(value); + + EXPECT_TRUE(pool.empty()); +} + +TEST(InternPoolTest, ReleaseByInterned) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + Slice interned = pool.acquire(value); + + EXPECT_FALSE(pool.empty()); + + pool.release(interned); + + EXPECT_TRUE(pool.empty()); +} + +TEST(InternPoolTest, AcquireAndReleaseEmpty) { + InternPool pool; + + FixedCapacityVector value = {}; + Slice interned = pool.acquire(value); + + EXPECT_TRUE(pool.empty()); + EXPECT_EQ(interned.begin(), nullptr); + EXPECT_EQ(interned.end(), nullptr); + + // Shouldn't crash to release an empty slice even if the pool is empty. + pool.release(value); + pool.release(interned); +} + +TEST(InternPoolTest, AcquireAndReleaseManyEqual) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + pool.acquire(value); + pool.acquire(value); + pool.acquire(value); + + EXPECT_FALSE(pool.empty()); + + pool.release(value); + EXPECT_FALSE(pool.empty()); + pool.release(value); + EXPECT_FALSE(pool.empty()); + pool.release(value); + EXPECT_TRUE(pool.empty()); + +#ifdef GTEST_HAS_DEATH_TEST + ASSERT_DEATH(pool.release(value), ""); +#endif +} + +TEST(InternPoolTest, AcquireAndReleaseManyDifferent) { + InternPool pool; + + FixedCapacityVector value1 = { 1, 3, 3, 7 }; + FixedCapacityVector value2 = { 4, 2, 0 }; + FixedCapacityVector value3 = { 9999999 }; + pool.acquire(value1); + pool.acquire(value2); + pool.acquire(value3); + + EXPECT_FALSE(pool.empty()); + + pool.release(value1); + EXPECT_FALSE(pool.empty()); + pool.release(value2); + EXPECT_FALSE(pool.empty()); + pool.release(value3); + EXPECT_TRUE(pool.empty()); +} + +#ifdef GTEST_HAS_DEATH_TEST +TEST(InternPoolTest, PanicsIfReleaseMissing) { + InternPool pool; + + FixedCapacityVector value = { 1, 3, 3, 7 }; + + ASSERT_DEATH(pool.release(value), ""); +} +#endif // GTEST_HAS_DEATH_TEST diff --git a/libs/utils/test/test_RefCountedMap.cpp b/libs/utils/test/test_RefCountedMap.cpp index f4e0cff65a..8340412816 100644 --- a/libs/utils/test/test_RefCountedMap.cpp +++ b/libs/utils/test/test_RefCountedMap.cpp @@ -77,6 +77,12 @@ TEST(RefCountedMapTest, ValueType_GetsValue) { EXPECT_EQ(v1const, 1); } +TEST(RefCountedMapTest, ValueType_CanReleaseNullValue) { + RefCountedMap map; + map.acquire(1); + map.release(1, [](ValueType& it) { ADD_FAILURE(); }); +} + #ifdef GTEST_HAS_DEATH_TEST TEST(RefCountedMapTest, ValueType_PanicsIfReleaseMissing) { RefCountedMap map; @@ -87,6 +93,12 @@ TEST(RefCountedMapTest, ValueType_PanicsIfGetsMissing) { RefCountedMap map; ASSERT_DEATH(map.get(1), ""); } + +TEST(RefCountedMapTest, ValueType_PanicsIfGetsNullValue) { + RefCountedMap map; + map.acquire(1); + ASSERT_DEATH(map.get(1), ""); +} #endif // GTEST_HAS_DEATH_TEST /* Plain pointer types */ @@ -149,6 +161,12 @@ TEST(RefCountedMapTest, PlainPointerType_GetsValue) { delete a1; } +TEST(RefCountedMapTest, PlainPointerType_CanReleaseNullValue) { + RefCountedMap map; + map.acquire(1); + map.release(1, [](ValueType& it) { ADD_FAILURE(); }); +} + #ifdef GTEST_HAS_DEATH_TEST TEST(RefCountedMapTest, PlainPointerType_PanicsIfReleaseMissing) { RefCountedMap map; @@ -159,6 +177,12 @@ TEST(RefCountedMapTest, PlainPointerType_PanicsIfGetsMissing) { RefCountedMap map; ASSERT_DEATH(map.get(1), ""); } + +TEST(RefCountedMapTest, PlainPointerType_PanicsIfGetsNullValue) { + RefCountedMap map; + map.acquire(1); + ASSERT_DEATH(map.get(1), ""); +} #endif // GTEST_HAS_DEATH_TEST /* Smart pointer types */ @@ -214,6 +238,12 @@ TEST(RefCountedMapTest, SmartPointerType_GetsValue) { EXPECT_EQ(v1const, 1); } +TEST(RefCountedMapTest, SmartPointerType_CanReleaseNullValue) { + RefCountedMap map; + map.acquire(1); + map.release(1, [](ValueType& it) { ADD_FAILURE(); }); +} + #ifdef GTEST_HAS_DEATH_TEST TEST(RefCountedMapTest, SmartPointerType_PanicsIfReleaseMissing) { RefCountedMap map; @@ -224,4 +254,10 @@ TEST(RefCountedMapTest, SmartPointerType_PanicsIfGetsMissing) { RefCountedMap map; ASSERT_DEATH(map.get(1), ""); } + +TEST(RefCountedMapTest, SmartPointerType_PanicsIfGetsNullValue) { + RefCountedMap map; + map.acquire(1); + ASSERT_DEATH(map.get(1), ""); +} #endif // GTEST_HAS_DEATH_TEST