Removed the cleanup list
It no longer has any users. If we need it later, we can add it back.
This saves one pointer of memory from `sizeof(upb_Arena)`.
Also, we now allow fuses if if the block allocators differ. This is made possible by cl/520144430, but was not take advantage of in that CL.
PiperOrigin-RevId: 520174588
diff --git a/upb/mem/arena.c b/upb/mem/arena.c
index 4b0bd45..43b254d 100644
--- a/upb/mem/arena.c
+++ b/upb/mem/arena.c
@@ -31,14 +31,6 @@
// Must be last.
#include "upb/port/def.inc"
-static uint32_t* upb_cleanup_pointer(uintptr_t cleanup_metadata) {
- return (uint32_t*)(cleanup_metadata & ~0x1);
-}
-
-static bool upb_cleanup_has_initial_block(uintptr_t cleanup_metadata) {
- return cleanup_metadata & 0x1;
-}
-
static uintptr_t upb_cleanup_metadata(uint32_t* cleanup,
bool has_initial_block) {
return (uintptr_t)cleanup | has_initial_block;
@@ -48,15 +40,9 @@
// Atomic only for the benefit of SpaceAllocated().
UPB_ATOMIC(_upb_MemBlock*) next;
uint32_t size;
- uint32_t cleanups;
// Data follows.
};
-typedef struct cleanup_ent {
- upb_CleanupFunc* cleanup;
- void* ud;
-} cleanup_ent;
-
static const size_t memblock_reserve =
UPB_ALIGN_UP(sizeof(_upb_MemBlock), UPB_MALLOC_ALIGN);
@@ -128,13 +114,10 @@
// Insert into linked list.
upb_Atomic_Init(&block->next, a->blocks);
block->size = (uint32_t)size;
- block->cleanups = 0;
upb_Atomic_Store(&a->blocks, block, memory_order_relaxed);
a->head.ptr = UPB_PTR_AT(block, memblock_reserve, char);
a->head.end = UPB_PTR_AT(block, size, char);
- a->cleanup_metadata = upb_cleanup_metadata(
- &block->cleanups, upb_cleanup_has_initial_block(a->cleanup_metadata));
UPB_POISON_MEMORY_REGION(a->head.ptr, a->head.end - a->head.ptr);
}
@@ -144,7 +127,7 @@
_upb_MemBlock* last_block = upb_Atomic_Load(&a->blocks, memory_order_relaxed);
size_t last_size = last_block != NULL ? last_block->size : 128;
size_t block_size = UPB_MAX(size, last_size * 2) + memblock_reserve;
- _upb_MemBlock* block = upb_malloc(a->block_alloc, block_size);
+ _upb_MemBlock* block = upb_malloc(upb_Arena_BlockAlloc(a), block_size);
if (!block) return false;
upb_Arena_AddBlock(a, block, block_size);
@@ -159,12 +142,13 @@
/* Public Arena API ***********************************************************/
-static upb_Arena* arena_initslow(void* mem, size_t n, upb_alloc* alloc) {
+static upb_Arena* upb_Arena_InitSlow(upb_alloc* alloc) {
const size_t first_block_overhead = sizeof(upb_Arena) + memblock_reserve;
upb_Arena* a;
/* We need to malloc the initial block. */
- n = first_block_overhead + 256;
+ char* mem;
+ size_t n = first_block_overhead + 256;
if (!alloc || !(mem = upb_malloc(alloc, n))) {
return NULL;
}
@@ -172,12 +156,11 @@
a = UPB_PTR_AT(mem, n - sizeof(*a), upb_Arena);
n -= sizeof(*a);
- a->block_alloc = alloc;
+ a->block_alloc = upb_Arena_MakeBlockAlloc(alloc, 0);
upb_Atomic_Init(&a->parent_or_count, _upb_Arena_TaggedFromRefcount(1));
upb_Atomic_Init(&a->next, NULL);
upb_Atomic_Init(&a->tail, a);
upb_Atomic_Init(&a->blocks, NULL);
- a->cleanup_metadata = upb_cleanup_metadata(NULL, false);
upb_Arena_AddBlock(a, mem, n);
@@ -200,7 +183,7 @@
n = UPB_ALIGN_DOWN(n, UPB_ALIGN_OF(upb_Arena));
if (UPB_UNLIKELY(n < sizeof(upb_Arena))) {
- return arena_initslow(mem, n, alloc);
+ return upb_Arena_InitSlow(alloc);
}
a = UPB_PTR_AT(mem, n - sizeof(*a), upb_Arena);
@@ -209,10 +192,9 @@
upb_Atomic_Init(&a->next, NULL);
upb_Atomic_Init(&a->tail, a);
upb_Atomic_Init(&a->blocks, NULL);
- a->block_alloc = alloc;
+ a->block_alloc = upb_Arena_MakeBlockAlloc(alloc, 1);
a->head.ptr = mem;
a->head.end = UPB_PTR_AT(mem, n - sizeof(*a), char);
- a->cleanup_metadata = upb_cleanup_metadata(NULL, true);
return a;
}
@@ -224,22 +206,13 @@
// Load first since arena itself is likely from one of its blocks.
upb_Arena* next_arena =
(upb_Arena*)upb_Atomic_Load(&a->next, memory_order_acquire);
+ upb_alloc* block_alloc = upb_Arena_BlockAlloc(a);
_upb_MemBlock* block = upb_Atomic_Load(&a->blocks, memory_order_relaxed);
while (block != NULL) {
- /* Load first since we are deleting block. */
+ // Load first since we are deleting block.
_upb_MemBlock* next_block =
upb_Atomic_Load(&block->next, memory_order_relaxed);
-
- if (block->cleanups > 0) {
- cleanup_ent* end = UPB_PTR_AT(block, block->size, void);
- cleanup_ent* ptr = end - block->cleanups;
-
- for (; ptr < end; ptr++) {
- ptr->cleanup(ptr->ud);
- }
- }
-
- upb_free(a->block_alloc, block);
+ upb_free(block_alloc, block);
block = next_block;
}
a = next_arena;
@@ -275,27 +248,6 @@
goto retry;
}
-bool upb_Arena_AddCleanup(upb_Arena* a, void* ud, upb_CleanupFunc* func) {
- cleanup_ent* ent;
- uint32_t* cleanups = upb_cleanup_pointer(a->cleanup_metadata);
-
- if (!cleanups || _upb_ArenaHas(a) < sizeof(cleanup_ent)) {
- if (!upb_Arena_AllocBlock(a, 128)) return false; /* Out of memory. */
- UPB_ASSERT(_upb_ArenaHas(a) >= sizeof(cleanup_ent));
- cleanups = upb_cleanup_pointer(a->cleanup_metadata);
- }
-
- a->head.end -= sizeof(cleanup_ent);
- ent = (cleanup_ent*)a->head.end;
- (*cleanups)++;
- UPB_UNPOISON_MEMORY_REGION(ent, sizeof(cleanup_ent));
-
- ent->cleanup = func;
- ent->ud = ud;
-
- return true;
-}
-
bool upb_Arena_Fuse(upb_Arena* a1, upb_Arena* a2) {
// SAFE IN THE PRESENCE OF FUSE/FREE RACES BUT NOT IN THE
// PRESENCE OF FUSE/FUSE RACES!!!
@@ -318,17 +270,16 @@
// that only refcounts can change once we have found the root. Because the
// threads doing the fuse must hold references, we can guarantee that no
// refcounts will reach zero concurrently.
+
upb_Arena* r1 = _upb_Arena_FindRoot(a1);
upb_Arena* r2 = _upb_Arena_FindRoot(a2);
if (r1 == r2) return true; // Already fused.
// Do not fuse initial blocks since we cannot lifetime extend them.
- if (upb_cleanup_has_initial_block(r1->cleanup_metadata)) return false;
- if (upb_cleanup_has_initial_block(r2->cleanup_metadata)) return false;
-
- // Only allow fuse with a common allocator
- if (r1->block_alloc != r2->block_alloc) return false;
+ // Any other fuse scenario is allowed.
+ if (upb_Arena_HasInitialBlock(r1)) return false;
+ if (upb_Arena_HasInitialBlock(r2)) return false;
uintptr_t r1_poc =
upb_Atomic_Load(&r1->parent_or_count, memory_order_acquire);
diff --git a/upb/mem/arena.h b/upb/mem/arena.h
index 3e1ddf1..4586b07 100644
--- a/upb/mem/arena.h
+++ b/upb/mem/arena.h
@@ -49,8 +49,6 @@
typedef struct upb_Arena upb_Arena;
-typedef void upb_CleanupFunc(void* context);
-
typedef struct {
char *ptr, *end;
} _upb_ArenaHead;
@@ -65,8 +63,6 @@
UPB_API upb_Arena* upb_Arena_Init(void* mem, size_t n, upb_alloc* alloc);
UPB_API void upb_Arena_Free(upb_Arena* a);
-UPB_API bool upb_Arena_AddCleanup(upb_Arena* a, void* ud,
- upb_CleanupFunc* func);
UPB_API bool upb_Arena_Fuse(upb_Arena* a, upb_Arena* b);
void* _upb_Arena_SlowMalloc(upb_Arena* a, size_t size);
diff --git a/upb/mem/arena_internal.h b/upb/mem/arena_internal.h
index 9b940dc..e5a81f0 100644
--- a/upb/mem/arena_internal.h
+++ b/upb/mem/arena_internal.h
@@ -37,23 +37,18 @@
struct upb_Arena {
_upb_ArenaHead head;
- /* Stores cleanup metadata for this arena.
- * - a pointer to the current cleanup counter.
- * - a boolean indicating if there is an unowned initial block. */
- uintptr_t cleanup_metadata;
- /* Allocator to allocate arena blocks. We are responsible for freeing these
- * when we are destroyed. */
- upb_alloc* block_alloc;
+ // upb_alloc* together with a low bit which signals if there is an initial
+ // block.
+ uintptr_t block_alloc;
- /* When multiple arenas are fused together, each arena points to a parent
- * arena (root points to itself). The root tracks how many live arenas
- * reference it.
- *
- * The low bit is tagged:
- * 0: pointer to parent
- * 1: count, left shifted by one
- */
+ // When multiple arenas are fused together, each arena points to a parent
+ // arena (root points to itself). The root tracks how many live arenas
+ // reference it.
+
+ // The low bit is tagged:
+ // 0: pointer to parent
+ // 1: count, left shifted by one
UPB_ATOMIC(uintptr_t) parent_or_count;
// All nodes that are fused together are in a singly-linked list.
@@ -99,6 +94,21 @@
return parent_or_count;
}
+UPB_INLINE upb_alloc* upb_Arena_BlockAlloc(upb_Arena* arena) {
+ return (upb_alloc*)(arena->block_alloc & ~0x1);
+}
+
+UPB_INLINE uintptr_t upb_Arena_MakeBlockAlloc(upb_alloc* alloc,
+ bool has_initial) {
+ uintptr_t alloc_uint = (uintptr_t)alloc;
+ UPB_ASSERT((alloc_uint & 1) == 0);
+ return alloc_uint | (has_initial ? 1 : 0);
+}
+
+UPB_INLINE bool upb_Arena_HasInitialBlock(upb_Arena* arena) {
+ return arena->block_alloc & 0x1;
+}
+
#include "upb/port/undef.inc"
#endif /* UPB_MEM_ARENA_INTERNAL_H_ */
diff --git a/upb/mem/arena_test.cc b/upb/mem/arena_test.cc
index b091997..87fadfd 100644
--- a/upb/mem/arena_test.cc
+++ b/upb/mem/arena_test.cc
@@ -17,38 +17,14 @@
namespace {
-static void decrement_int(void* ptr) {
- int* iptr = static_cast<int*>(ptr);
- (*iptr)--;
-}
-
TEST(ArenaTest, ArenaFuse) {
- int i1 = 5;
- int i2 = 5;
- int i3 = 5;
- int i4 = 5;
-
upb_Arena* arena1 = upb_Arena_New();
upb_Arena* arena2 = upb_Arena_New();
- upb_Arena_AddCleanup(arena1, &i1, decrement_int);
- upb_Arena_AddCleanup(arena2, &i2, decrement_int);
-
EXPECT_TRUE(upb_Arena_Fuse(arena1, arena2));
- upb_Arena_AddCleanup(arena1, &i3, decrement_int);
- upb_Arena_AddCleanup(arena2, &i4, decrement_int);
-
upb_Arena_Free(arena1);
- EXPECT_EQ(5, i1);
- EXPECT_EQ(5, i2);
- EXPECT_EQ(5, i3);
- EXPECT_EQ(5, i4);
upb_Arena_Free(arena2);
- EXPECT_EQ(4, i1);
- EXPECT_EQ(4, i2);
- EXPECT_EQ(4, i3);
- EXPECT_EQ(4, i4);
}
/* Do nothing allocator for testing */
@@ -56,19 +32,18 @@
size_t size) {
return upb_alloc_global.func(alloc, ptr, oldsize, size);
}
-ABSL_CONST_INIT upb_alloc test_alloc = {&TestAllocFunc};
TEST(ArenaTest, FuseWithInitialBlock) {
char buf1[1024];
char buf2[1024];
upb_Arena* arenas[] = {upb_Arena_Init(buf1, 1024, &upb_alloc_global),
upb_Arena_Init(buf2, 1024, &upb_alloc_global),
- upb_Arena_Init(NULL, 0, &test_alloc),
upb_Arena_Init(NULL, 0, &upb_alloc_global)};
int size = sizeof(arenas) / sizeof(arenas[0]);
for (int i = 0; i < size; ++i) {
for (int j = 0; j < size; ++j) {
if (i == j) {
+ // Fuse to self is always allowed.
EXPECT_TRUE(upb_Arena_Fuse(arenas[i], arenas[j]));
} else {
EXPECT_FALSE(upb_Arena_Fuse(arenas[i], arenas[j]));
diff --git a/upb/test/test_cpp.cc b/upb/test/test_cpp.cc
index eb77872..6e8bb1d 100644
--- a/upb/test/test_cpp.cc
+++ b/upb/test/test_cpp.cc
@@ -65,67 +65,6 @@
EXPECT_EQ(oneof_count, md.oneof_count());
}
-TEST(Cpp, Arena) {
- int n = 100000;
-
- struct Decrementer {
- Decrementer(int* _p) : p(_p) {}
- ~Decrementer() { (*p)--; }
- int* p;
- };
-
- {
- upb::Arena arena;
- for (int i = 0; i < n; i++) {
- arena.Own(new Decrementer(&n));
-
- // Intersperse allocation and ensure we can write to it.
- int* val = static_cast<int*>(upb_Arena_Malloc(arena.ptr(), sizeof(int)));
- *val = i;
- }
-
- // Test a large allocation.
- upb_Arena_Malloc(arena.ptr(), 1000000);
- }
- EXPECT_EQ(0, n);
-
- {
- // Test fuse.
- upb::Arena arena1;
- upb::Arena arena2;
-
- arena1.Fuse(arena2);
-
- upb_Arena_Malloc(arena1.ptr(), 10000);
- upb_Arena_Malloc(arena2.ptr(), 10000);
- }
-}
-
-TEST(Cpp, InlinedArena) {
- int n = 100000;
-
- struct Decrementer {
- Decrementer(int* _p) : p(_p) {}
- ~Decrementer() { (*p)--; }
- int* p;
- };
-
- {
- upb::InlinedArena<1024> arena;
- for (int i = 0; i < n; i++) {
- arena.Own(new Decrementer(&n));
-
- // Intersperse allocation and ensure we can write to it.
- int* val = static_cast<int*>(upb_Arena_Malloc(arena.ptr(), sizeof(int)));
- *val = i;
- }
-
- // Test a large allocation.
- upb_Arena_Malloc(arena.ptr(), 1000000);
- }
- EXPECT_EQ(0, n);
-}
-
TEST(Cpp, InlinedArena2) {
upb::InlinedArena<64> arena;
upb_Arena_Malloc(arena.ptr(), sizeof(int));
diff --git a/upb/test/test_generated_code.cc b/upb/test/test_generated_code.cc
index e2db7d9..99d5b46 100644
--- a/upb/test/test_generated_code.cc
+++ b/upb/test/test_generated_code.cc
@@ -854,95 +854,11 @@
(*iptr)--;
}
-TEST(GeneratedCode, ArenaFuse) {
- int i1 = 5;
- int i2 = 5;
- int i3 = 5;
- int i4 = 5;
-
- upb_Arena* arena1 = upb_Arena_New();
- upb_Arena* arena2 = upb_Arena_New();
-
- upb_Arena_AddCleanup(arena1, &i1, decrement_int);
- upb_Arena_AddCleanup(arena2, &i2, decrement_int);
-
- EXPECT_TRUE(upb_Arena_Fuse(arena1, arena2));
-
- upb_Arena_AddCleanup(arena1, &i3, decrement_int);
- upb_Arena_AddCleanup(arena2, &i4, decrement_int);
-
- upb_Arena_Free(arena1);
- EXPECT_EQ(5, i1);
- EXPECT_EQ(5, i2);
- EXPECT_EQ(5, i3);
- EXPECT_EQ(5, i4);
- upb_Arena_Free(arena2);
- EXPECT_EQ(4, i1);
- EXPECT_EQ(4, i2);
- EXPECT_EQ(4, i3);
- EXPECT_EQ(4, i4);
-}
-
/* Do nothing allocator for testing */
static void* test_allocfunc(upb_alloc* alloc, void* ptr, size_t oldsize,
size_t size) {
return upb_alloc_global.func(alloc, ptr, oldsize, size);
}
-upb_alloc test_alloc = {&test_allocfunc};
-
-TEST(GeneratedCode, FuseWithInitialBlock) {
- char buf1[1024];
- char buf2[1024];
- upb_Arena* arenas[] = {upb_Arena_Init(buf1, 1024, &upb_alloc_global),
- upb_Arena_Init(buf2, 1024, &upb_alloc_global),
- upb_Arena_Init(NULL, 0, &test_alloc),
- upb_Arena_Init(NULL, 0, &upb_alloc_global)};
- int size = sizeof(arenas) / sizeof(arenas[0]);
- for (int i = 0; i < size; ++i) {
- for (int j = 0; j < size; ++j) {
- if (i == j) {
- EXPECT_TRUE(upb_Arena_Fuse(arenas[i], arenas[j]));
- } else {
- EXPECT_FALSE(upb_Arena_Fuse(arenas[i], arenas[j]));
- }
- }
- }
-
- for (int i = 0; i < size; ++i) upb_Arena_Free(arenas[i]);
-}
-
-TEST(GeneratedCode, ArenaDecode) {
- // Tests against a bug that previously existed when passing an arena to
- // upb_decode().
- char large_string[1024] = {0};
- upb_StringView large_string_view = {large_string, sizeof(large_string)};
- upb_Arena* tmp = upb_Arena_New();
-
- protobuf_test_messages_proto3_TestAllTypesProto3* msg =
- protobuf_test_messages_proto3_TestAllTypesProto3_new(tmp);
-
- protobuf_test_messages_proto3_TestAllTypesProto3_set_optional_bytes(
- msg, large_string_view);
-
- upb_StringView serialized;
- serialized.data = protobuf_test_messages_proto3_TestAllTypesProto3_serialize(
- msg, tmp, &serialized.size);
-
- upb_Arena* arena = upb_Arena_New();
- // Parse the large payload, forcing an arena block to be allocated. This used
- // to corrupt the cleanup list, preventing subsequent upb_Arena_AddCleanup()
- // calls from working properly.
- protobuf_test_messages_proto3_TestAllTypesProto3_parse(
- serialized.data, serialized.size, arena);
-
- int i1 = 5;
- upb_Arena_AddCleanup(arena, &i1, decrement_int);
- EXPECT_EQ(5, i1);
- upb_Arena_Free(arena);
- EXPECT_EQ(4, i1);
-
- upb_Arena_Free(tmp);
-}
TEST(GeneratedCode, ArenaUnaligned) {
char buf1[1024];
diff --git a/upb/upb.hpp b/upb/upb.hpp
index c99ac33..78f1c77 100644
--- a/upb/upb.hpp
+++ b/upb/upb.hpp
@@ -77,14 +77,6 @@
upb_Arena* ptr() const { return ptr_.get(); }
- // Add a cleanup function to run when the arena is destroyed.
- // Returns false on out-of-memory.
- template <class T>
- bool Own(T* obj) {
- return upb_Arena_AddCleanup(ptr_.get(), obj,
- [](void* obj) { delete static_cast<T*>(obj); });
- }
-
void Fuse(Arena& other) { upb_Arena_Fuse(ptr(), other.ptr()); }
protected:
diff --git a/upb/wire/decode.c b/upb/wire/decode.c
index e1697fa..ed2ee98 100644
--- a/upb/wire/decode.c
+++ b/upb/wire/decode.c
@@ -1285,7 +1285,6 @@
_upb_MemBlock* blocks =
upb_Atomic_Load(&decoder->arena.blocks, memory_order_relaxed);
arena->head = decoder->arena.head;
- arena->cleanup_metadata = decoder->arena.cleanup_metadata;
upb_Atomic_Store(&arena->blocks, blocks, memory_order_relaxed);
return decoder->status;
}
@@ -1316,7 +1315,6 @@
_upb_MemBlock* blocks = upb_Atomic_Load(&arena->blocks, memory_order_relaxed);
decoder.arena.head = arena->head;
decoder.arena.block_alloc = arena->block_alloc;
- decoder.arena.cleanup_metadata = arena->cleanup_metadata;
upb_Atomic_Init(&decoder.arena.blocks, blocks);
return upb_Decoder_Decode(&decoder, buf, msg, l, arena);
