From 73489a9339f234270fb6026e74a616300fb520d9 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Tue, 28 Mar 2023 17:03:08 -0700 Subject: [PATCH] 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 --- upb/mem/arena.c | 77 ++++++------------------------ upb/mem/arena.h | 4 -- upb/mem/arena_internal.h | 44 ++++++++++------- upb/mem/arena_test.cc | 27 +---------- upb/test/test_cpp.cc | 61 ------------------------ upb/test/test_generated_code.cc | 84 --------------------------------- upb/upb.hpp | 8 ---- upb/wire/decode.c | 2 - 8 files changed, 42 insertions(+), 265 deletions(-) diff --git a/upb/mem/arena.c b/upb/mem/arena.c index 4b0bd457c4..43b254df0f 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 @@ struct _upb_MemBlock { // 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 @@ static void upb_Arena_AddBlock(upb_Arena* a, void* ptr, size_t size) { // 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 @@ static bool upb_Arena_AllocBlock(upb_Arena* a, size_t size) { _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 @@ void* _upb_Arena_SlowMalloc(upb_Arena* a, size_t size) { /* 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 @@ static upb_Arena* arena_initslow(void* mem, size_t n, upb_alloc* alloc) { 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 @@ upb_Arena* upb_Arena_Init(void* mem, size_t n, upb_alloc* alloc) { 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_Arena* upb_Arena_Init(void* mem, size_t n, upb_alloc* alloc) { 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 @@ static void arena_dofree(upb_Arena* a) { // 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 @@ retry: 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 @@ bool upb_Arena_Fuse(upb_Arena* a1, upb_Arena* a2) { // 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 3e1ddf1401..4586b077e4 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 @@ extern "C" { 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 9b940dcfc8..e5a81f0d1a 100644 --- a/upb/mem/arena_internal.h +++ b/upb/mem/arena_internal.h @@ -37,23 +37,18 @@ typedef struct _upb_MemBlock _upb_MemBlock; 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; - - /* 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_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 UPB_ATOMIC(uintptr_t) parent_or_count; // All nodes that are fused together are in a singly-linked list. @@ -99,6 +94,21 @@ UPB_INLINE uintptr_t _upb_Arena_TaggedFromPointer(upb_Arena* a) { 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 b09199726d..87fadfd60a 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(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 @@ extern "C" void* TestAllocFunc(upb_alloc* alloc, void* ptr, size_t oldsize, 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 eb77872cd3..6e8bb1df86 100644 --- a/upb/test/test_cpp.cc +++ b/upb/test/test_cpp.cc @@ -65,67 +65,6 @@ TEST(Cpp, Iteration) { 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(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(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 e2db7d97dc..99d5b467bb 100644 --- a/upb/test/test_generated_code.cc +++ b/upb/test/test_generated_code.cc @@ -854,95 +854,11 @@ static void decrement_int(void* ptr) { (*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 c99ac3342b..78f1c77e67 100644 --- a/upb/upb.hpp +++ b/upb/upb.hpp @@ -77,14 +77,6 @@ class Arena { 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 - bool Own(T* obj) { - return upb_Arena_AddCleanup(ptr_.get(), obj, - [](void* obj) { delete static_cast(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 e1697fa428..ed2ee98f68 100644 --- a/upb/wire/decode.c +++ b/upb/wire/decode.c @@ -1285,7 +1285,6 @@ static upb_DecodeStatus upb_Decoder_Decode(upb_Decoder* const decoder, _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_DecodeStatus upb_Decode(const char* buf, size_t size, void* msg, _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);