Skip to content

Commit

Permalink
Delete the page pool allocator
Browse files Browse the repository at this point in the history 
The page pool allocator leaks memory when used in a threaded context
(amazon-ion#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.
  • Loading branch information
@sadderchris
sadderchris committed Feb 3, 2022
1 parent 4f9fbd1 commit bcfcbd2
Show file tree
Hide file tree
Showing 5 changed files with 4 additions and 150 deletions.
42 changes: 0 additions & 42 deletions ionc/ion_alloc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
99 changes: 4 additions & 95 deletions ionc/ion_allocation.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -178,18 +178,8 @@ 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;
}
}
new_block = (ION_ALLOCATION_CHAIN *)ion_xalloc(alloc_size);
new_block->size = alloc_size;

// see if we suceeded
if (!new_block) return NULL;
Expand All @@ -208,91 +198,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);
}
ion_xfree(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++;
}
return;
}



#ifdef MEM_DEBUG

long malloc_inuse = 0;
Expand Down
4 changes: 0 additions & 4 deletions tools/ionizer/ionizer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 0 additions & 1 deletion tools/ionizer/ionizer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down
8 changes: 0 additions & 8 deletions tools/ionizer/ionizer_args.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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" },
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit bcfcbd2

Please sign in to comment.