From 4cc1349e7a553bd39aa880e97f1e793b7e77847d Mon Sep 17 00:00:00 2001 From: Chris Sadler Date: Mon, 20 Dec 2021 13:28:53 -0800 Subject: [PATCH] Delete the page pool allocator The page pool allocator leaks memory when used in a threaded context (#264). While we could invest in making this thread-safe by using proper concurrency primitives, it's not clear that the page pool allocator is actually providing any benefit right now. There are no benchmarks to indicate that it is, and any sane allocator should already be doing something like this behind the scenes (in a thread-safe way to boot). This nukes the page pool code. We can perform some perf tests and benchmarks to determine if it's really worth it to reintroduce something like it in the future. --- ionc/ion_alloc.h | 42 -------------- ionc/ion_allocation.c | 105 +++-------------------------------- tools/ionizer/ionizer.c | 4 -- tools/ionizer/ionizer.h | 1 - tools/ionizer/ionizer_args.c | 8 --- 5 files changed, 8 insertions(+), 152 deletions(-) diff --git a/ionc/ion_alloc.h b/ionc/ion_alloc.h index c20752e3..c8e86edf 100644 --- a/ionc/ion_alloc.h +++ b/ionc/ion_alloc.h @@ -131,48 +131,6 @@ typedef struct _ion_allocation_chain DBG_ION_ALLOCATION_CHAIN; #endif -// -// structures and functions for the ion allocation page pool -// this is a list of free pages which are used by the various -// pools created by the Ion routines on behalf of the various -// objects, such as the reader, writer, or catalog. -// -// THIS IS NOT THREAD SAFE - (which could be corrected) -// - -typedef struct _ion_alloc_page ION_ALLOC_PAGE; -typedef struct _ion_alloc_page_list ION_ALLOC_PAGE_LIST; - -struct _ion_alloc_page -{ - ION_ALLOC_PAGE *next; -}; - -struct _ion_alloc_page_list -{ - SIZE page_size; - int page_count; - int free_page_limit; - ION_ALLOC_PAGE *head; -}; - -#define ION_ALLOC_PAGE_POOL_NO_LIMIT (-1) -#define ION_ALLOC_PAGE_POOL_DEFAULT_LIMIT (16) -#define ION_ALLOC_PAGE_MIN_SIZE (ALIGN_SIZE(sizeof(ION_ALLOCATION_CHAIN))) -#define ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE (DEFAULT_BLOCK_SIZE) -#define ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE (-1) - -GLOBAL THREAD_LOCAL_STORAGE ION_ALLOC_PAGE_LIST g_ion_alloc_page_list -#ifdef INIT_STATICS -= { ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE, 0, ION_ALLOC_PAGE_POOL_DEFAULT_LIMIT, NULL } -#endif -; - -ION_API_EXPORT void ion_initialize_page_pool (SIZE page_size, int free_page_limit); -ION_API_EXPORT void ion_release_page_pool (void); - -ION_ALLOC_PAGE *_ion_alloc_page (void); -void _ion_release_page (ION_ALLOC_PAGE *page); void *_ion_alloc_owner (SIZE len); void *_ion_alloc_with_owner(hOWNER owner, SIZE length); diff --git a/ionc/ion_allocation.c b/ionc/ion_allocation.c index e8bb9e02..88c8c782 100644 --- a/ionc/ion_allocation.c +++ b/ionc/ion_allocation.c @@ -141,7 +141,7 @@ void *_ion_alloc_with_owner_helper(ION_ALLOCATION_CHAIN *powner, SIZE request_le pblock = _ion_alloc_block(length); if (!pblock) return NULL; - if (pblock->size > g_ion_alloc_page_list.page_size && powner->head != NULL) { + if (pblock->size > DEFAULT_BLOCK_SIZE && powner->head != NULL) { // this is an oversized block, so don't put it // at the front since it will be full and we'll // have wasted the freespace in the current @@ -178,29 +178,21 @@ ION_ALLOCATION_CHAIN *_ion_alloc_block(SIZE min_needed) ION_ALLOCATION_CHAIN *new_block; SIZE alloc_size = min_needed + ALIGN_SIZE(sizeof(ION_ALLOCATION_CHAIN)); // subtract out the block[1] - if (alloc_size > g_ion_alloc_page_list.page_size) { - // it's an oversize block - we'll ask the system for this one - new_block = (ION_ALLOCATION_CHAIN *)ion_xalloc(alloc_size); - new_block->size = alloc_size; - } - else { - // it's a normal size block - go out to the block pool for it - new_block = (ION_ALLOCATION_CHAIN *)_ion_alloc_page(); - if (new_block) { - new_block->size = g_ion_alloc_page_list.page_size; - } - } + if (alloc_size < DEFAULT_BLOCK_SIZE) alloc_size = DEFAULT_BLOCK_SIZE; + + new_block = (ION_ALLOCATION_CHAIN *)ion_xalloc(alloc_size); // see if we suceeded if (!new_block) return NULL; - + + new_block->size = alloc_size; new_block->next = NULL; new_block->head = NULL; new_block->position = ION_ALLOC_BLOCK_TO_USER_PTR(new_block); new_block->limit = ((BYTE*)new_block) + new_block->size; - assert(new_block->position == ((BYTE *)(&new_block->limit) + ALIGN_SIZE(sizeof(new_block->limit)))); + assert(new_block->position == ((BYTE *)ALIGN_PTR((BYTE *)(&new_block->limit) + ALIGN_SIZE(sizeof(new_block->limit))))); return new_block; } @@ -208,91 +200,10 @@ ION_ALLOCATION_CHAIN *_ion_alloc_block(SIZE min_needed) void _ion_free_block(ION_ALLOCATION_CHAIN *pblock) { if (!pblock) return; - if (pblock->size > g_ion_alloc_page_list.page_size) { - ion_xfree(pblock); - } - else { - _ion_release_page((ION_ALLOC_PAGE *)pblock); - } - return; -} - -void ion_initialize_page_pool(SIZE page_size, int free_page_limit) -{ - // TODO This is always true, causing this function to do nothing, because g_ion_alloc_page_list.page_size - // is statically initialized to ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE. The original intent of this check - // (when it was != ) was to attempt to determine whether the page pool had already been initialized, - // however the check was insufficient. The problem of how to safely allow users to configure the page - // pool exactly once needs to be solved. See https://github.com/amzn/ion-c/issues/242 . - if (g_ion_alloc_page_list.page_size == ION_ALLOC_PAGE_POOL_DEFAULT_PAGE_SIZE) - { - return; - } - - // we need a min size to hold the pointers we use to maintain the page list - if (page_size < ION_ALLOC_PAGE_MIN_SIZE) - { - page_size = ION_ALLOC_PAGE_MIN_SIZE; - } - g_ion_alloc_page_list.page_size = page_size; - g_ion_alloc_page_list.free_page_limit = free_page_limit; - - // TODO: should we pre-allocate the pages? - - return; -} - -void ion_release_page_pool(void) -{ - ION_ALLOC_PAGE *page; - - while ((page = g_ion_alloc_page_list.head) != NULL) { - g_ion_alloc_page_list.head = page->next; - ion_xfree(page); - g_ion_alloc_page_list.page_count--; - } - ASSERT(g_ion_alloc_page_list.page_count == 0); - - // here we mark the page size to indicate the pool is inactive - g_ion_alloc_page_list.page_size = ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE; - - return; -} - -ION_ALLOC_PAGE *_ion_alloc_page(void) -{ - ION_ALLOC_PAGE *page; - - if ((page = g_ion_alloc_page_list.head) != NULL) { - g_ion_alloc_page_list.head = page->next; - g_ion_alloc_page_list.page_count--; - } - else { - ASSERT(g_ion_alloc_page_list.page_size != ION_ALLOC_PAGE_POOL_PAGE_SIZE_NONE); - page = ion_xalloc(g_ion_alloc_page_list.page_size); - } - return page; -} - -void _ion_release_page(ION_ALLOC_PAGE *page) -{ - if (page == NULL) return; - - if (g_ion_alloc_page_list.free_page_limit != ION_ALLOC_PAGE_POOL_NO_LIMIT - && g_ion_alloc_page_list.page_count >= g_ion_alloc_page_list.free_page_limit - ) { - ion_xfree(page); - } - else { - page->next = g_ion_alloc_page_list.head; - g_ion_alloc_page_list.head = page; - g_ion_alloc_page_list.page_count++; - } + ion_xfree(pblock); return; } - - #ifdef MEM_DEBUG long malloc_inuse = 0; diff --git a/tools/ionizer/ionizer.c b/tools/ionizer/ionizer.c index 5f706b0a..ad54fb8a 100644 --- a/tools/ionizer/ionizer.c +++ b/tools/ionizer/ionizer.c @@ -56,10 +56,6 @@ int main(int argc, char **argv) g_writer_options.output_as_binary = g_ionizer_write_binary; g_writer_options.escape_all_non_ascii = g_ionizer_ascii_only; - if (g_ionizer_pool_page_size > 0) { - ion_initialize_page_pool(g_ionizer_pool_page_size, 10); - } - // set up our debug options if (g_ionizer_flush_each) g_writer_options.flush_every_value = TRUE; if (g_ionizer_dump_args) ionizer_dump_arg_globals(); diff --git a/tools/ionizer/ionizer.h b/tools/ionizer/ionizer.h index cd48d9c4..38ee9222 100644 --- a/tools/ionizer/ionizer.h +++ b/tools/ionizer/ionizer.h @@ -57,7 +57,6 @@ IZ_GLOBAL BOOL g_ionizer_debug IZ_INITTO(FALSE); IZ_GLOBAL BOOL g_ionizer_dump_args IZ_INITTO(FALSE); IZ_GLOBAL int g_ionizer_symtab_version IZ_INITTO(0); -IZ_GLOBAL int g_ionizer_pool_page_size IZ_INITTO(-1); // IZ_GLOBAL char g_ionizer_symtab_name[MAX_FILE_NAME_LEN + 1] IZ_INITTO({0}); // IZ_GLOBAL char g_ionizer_catalog[MAX_FILE_NAME_LEN + 1] IZ_INITTO({0}); diff --git a/tools/ionizer/ionizer_args.c b/tools/ionizer/ionizer_args.c index 2c87f91f..24196122 100644 --- a/tools/ionizer/ionizer_args.c +++ b/tools/ionizer/ionizer_args.c @@ -31,7 +31,6 @@ BOOL set_name(OC *pcur); BOOL set_symbol_table(OC *pcur); BOOL set_catalog_name(OC *pcur); BOOL set_version(OC *pcur); -BOOL set_pagesize(OC *pcur); BOOL set_write_binary(OC *pcur); BOOL set_write_symtab(OC *pcur); BOOL set_ugly(OC *pcur); @@ -54,7 +53,6 @@ OPT_DEF inionizer_options[] = { { ot_int, 't', "trace", TRUE, FALSE, set_trace, "turns on trace options 1:args,2:parse state,3:parse fns,4:tokens,5:calls,6:time" }, { ot_string, 'n', "name", FALSE, FALSE, set_name, "sets the output symbol table name" }, { ot_string, 'o', "output", FALSE, FALSE, set_output, "sets the output format: ugly, binary, pretty, counts, none(scan only), types(counts)" }, - { ot_int, 'p', "pagesize", TRUE, FALSE, set_pagesize, "set the page size of the memory pool" }, { ot_string, 's', "symbol_table", FALSE, FALSE, set_symbol_table, "symbol table file for the writer (uses newest version)" }, { ot_none, 'u', NULL, FALSE, FALSE, set_ugly, "sets the output format to ugly" }, { ot_int, 'v', "version", FALSE, FALSE, set_version, "set the output symbol tables version" }, @@ -141,12 +139,6 @@ BOOL set_version(OC *pcur) { g_ionizer_symtab_version = atoi(val); return TRUE; } -BOOL set_pagesize(OC *pcur) { - char * val = opt_get_arg(pcur); - if (!val) return FALSE; - g_ionizer_pool_page_size = atoi(val); - return TRUE; -} BOOL set_write_binary(OC *pcur) { g_ionizer_write_binary = TRUE;