Try making all memory pool and freelist code off by default, or clear-on-alloc
We've got mempool.c, and we've got freelists in buf_t. After recent shenanigans, it seems like a good idea to turn them off-by-default to see if any platforms actually need them, and have them on only when needed.
Additionally, we should look to see whether there's any performance hit that matters for making them get cleared before they're released.
Activity
changed milestone to %Tor: 0.2.5.x-final
Just merged Andrea's "bug11476" branch to 0.2.5.x to fix #11623 (moved). As it stood, it only implemented #11623 (moved) (fixing --disable-buf-freelists), but that's a good start.
Replying to nickm:
Suggestion:
- Rather than scattering ENABLE_MEMPOOLS items through our code, why not instead make the code in mempool.h just wrap malloc_zero() and free() when we're running in with mempools disabled? That way, the rest of the code stays pretty.
...actually, because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().
Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?
-
because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().
That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."
Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?
I think that approach is okay too. The thing I don't want to have happen is multiple instances of
#ifdef ENABLE_MEMPOOLS foo(); #else bar(); #endifscattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.
Replying to nickm:
because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().
That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."
Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?
I think that approach is okay too. The thing I don't want to have happen is multiple instances of
{{{ #ifdef ENABLE_MEMPOOLS foo(); #else bar(); #endif }}}
scattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.
Okay, fine by me.
Replying to nickm:
because mp_pool_get() takes a cell pool as an argument, so I'd also have to leave 'static mp_pool_t *cell_pool = NULL;' in relay.c outside the #ifdef if I were keeping the call to mp_pool_get() but making it wrap tor_malloc_zero().
That's not so bad IMO; you'd just need to have mp_pool_t be semantically "This is a memory pool if ENABLE_MEMPOOLS is set; this is a placeholder object if it isn't."
Counter-suggestion: replace mp_pool_get() calls in relay.c with relay_alloc_cell() or something like that, which is #defined to either mp_pool_get(cell_pool) or tor_malloc_zero() as appropriate?
I think that approach is okay too. The thing I don't want to have happen is multiple instances of
{{{ #ifdef ENABLE_MEMPOOLS foo(); #else bar(); #endif }}}
scattered throughout the codebase. That's a recipe for hard-to-maintain code IMO. Ideally, when there's an ENABLE_foo, its effects should be isolated by a function abstraction at some level, though IMO the exact level is not super critical.
Updated branch eliminates the #ifdefs around the alloc/free stuff in relay.c How do you feel about the init_cell_pool()/cleanup_cell_pool() calls in main.c and circuitlist.c?
mentioned in issue #11477 (moved)