HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

New malloc implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mallocimplementationnew

Problem

I've written a new malloc implementation similar to dlmalloc and was hoping for feedback on it.

The goals for this library are:

  • Easy to read and maintain



  • Be more memory conserving



  • High efficiency and good performance



  • Portability



The documentation is here and the code could be found here.

```
/ mallocstate config begin ****/

/*!
* Decide to use or not use locks
*/
#if !defined(AKMALLOC_USE_LOCKS) || AKMALLOC_USE_LOCKS
# define AK_MALLOCSTATE_USE_LOCKS
#endif

#if defined(AK_MALLOCSTATE_USE_LOCKS)
# define AK_SLAB_USE_LOCKS
# define AK_CA_USE_LOCKS
# define AKMALLOC_LOCK_DEFINE(nm) ak_spinlock nm
# define AKMALLOC_LOCK_INIT(lk) ak_spinlock_init((lk))
# define AKMALLOC_LOCK_ACQUIRE(lk) ak_spinlock_acquire((lk))
# define AKMALLOC_LOCK_RELEASE(lk) ak_spinlock_release((lk))
#else
# define AKMALLOC_LOCK_DEFINE(nm)
# define AKMALLOC_LOCK_INIT(lk)
# define AKMALLOC_LOCK_ACQUIRE(lk)
# define AKMALLOC_LOCK_RELEASE(lk)
#endif

#if !defined(AK_COALESCE_SEGMENT_GRANULARITY)
# define AK_COALESCE_SEGMENT_GRANULARITY (((size_t)1) LOCKED))
# define AK_SLAB_LOCK_ACQUIRE(root) ak_spinlock_acquire(ak_as_ptr((root)->LOCKED))
# define AK_SLAB_LOCK_RELEASE(root) ak_spinlock_release(ak_as_ptr((root)->LOCKED))
#else
# define AK_SLAB_LOCK_DEFINE(nm)
# define AK_SLAB_LOCK_INIT(root)
# define AK_SLAB_LOCK_ACQUIRE(root)
# define AK_SLAB_LOCK_RELEASE(root)
#endif

struct ak_slab_tag
{
ak_slab* fd;
ak_slab* bk;
ak_slab_root* root;
ak_bitset512 avail;
void* _unused;
};

/*!
* Slab allocator
*/
struct ak_slab_root_tag
{
ak_u32 sz; /**bk->fd = (sU->fd); \
sU->fd->bk = (sU->bk); \
sU->fd = sU->bk = AK_NULLPTR; \
} while (0)

#define ak_slab_link_fd(slab, fwd) \
do { \
ak_slab* const sLF = (slab); \
ak_slab* const fLF = (fwd); \
sLF->fd =

Solution

It's obvious that you've spent a lot of time on this project. As
far as I can tell the things you've done are to optimize the size
of the code and the speed of execution. Optimization sometimes
leads to code that is less maintainable.

Using your initials isn't a bad idea, but I would use ak__
rather than ak_ as the prefix. That way it's clearer that it is a
prefix rather than an abbreviation. It's obvious that you're
trying to work around the fact that the C language doesn't
support name spaces.

I do wonder why you felt it was necessary to create your own
ASSERTS rather than using the standard ASSERT().

Word Size

The size of unsigned int is compiler/machine dependent, the
typedef name ak_u32 is misleading, see this StackOverflow question.
This can lead to portability problems and your alignment isn't
guarenteed.

Lower Case Macro Names

Using lower case macro names can be misleading, macros are
generally all upper case. I would be expecting all the lower
case macros to be functions.

#define ak_slab_unlink(slab)               \
  do {                                     \
    ak_slab* const sU = (slab);            \
    sU->bk->fd = (sU->fd);                 \
    sU->fd->bk = (sU->bk);                 \
    sU->fd = sU->bk = AK_NULLPTR;          \
  } while (0)


I as a developer would be very surprised if I had to maintain
this code.

Inline Functions

Inline functions were originally created for C++, not C. Inline
functions were created to replace macros. Is this library C or C++?

Informative Variable and Field Names

You have the struct

struct ak_slab_tag
{
    ak_slab*      fd;
    ak_slab*      bk;
    ak_slab_root* root;
    ak_bitset512  avail;
    void*         _unused;
};


I might be able to figure out what root, avail and _unused
mean (avail and _unused are pretty clear), but I have no clue
what fd or bk are. To me fd might be a File Descriptor. I have
the same problem with r in the following function.

static void ak_slab_release_pages(ak_slab_root* root, ak_slab* s, ak_u32 numtofree)
{
    ak_slab* const r = s;
    ak_slab* next = AK_NULLPTR;
    s = s->fd;
    for (ak_u32 ct = 0; ct fd;
        }
        ak_slab_unlink(s);
        ak_os_free(s, AKMALLOC_DEFAULT_PAGE_SIZE);
        s = next;
    }
}


NULLPTR

As a C programmer I know what NULL is. If I wanted to define
my own NULL I would probably do it this way

#define AK_NULLPTR NULL


Better yet, I would just use NULL.

Macro Nesting Duplicates Code

If I act as the C PreProcessor what I get for this macro

#define ak_slab_link(slab, fwd, back)      \
  do {                                     \
    ak_slab* const sL = (slab);            \
    ak_slab* const fL = (fwd);             \
    ak_slab* const bL = (back);            \
    ak_slab_link_bk(sL, bL);               \
    ak_slab_link_fd(sL, fL);               \
  } while (0)


is the following:

#define ak_slab_link(slab, fwd, back)    
  do {                                  
    ak_slab* const sL = (slab);        
    ak_slab* const fL = (fwd);        
    ak_slab* const bL = (back);      
    ak_slab_link_bk(sL, bL);        
    do {                             
      ak_slab* const sLB = (slab);  
      ak_slab* const bLB = (back);      
      sLB->bk = bLB;                   
      bLB->fd = sLB;                  
    } while (0);
    do {                                     
      ak_slab* const sLF = (slab);          
      ak_slab* const fLF = (fwd);          
      sLF->fd = fLF;                      
      fLF->bk = sLF;                     
    } while (0);
  } while (0)


When it could simply be:

sLB->bk = (slab);
    bLB->fd = (back);                  
    sLF->fd = (fwd);                      
    fLF->bk = (slab);


Please note that the only place ak_slab_link_bk()
and ak_slab_link_fd() are used is in the ak_slab_link() macro.

The ak_slab_link() macro is used effectively in several
functions.

If I needed to maintain ak_slab_link() with the current
implementation it is quite confusing.

Code Snippets

#define ak_slab_unlink(slab)               \
  do {                                     \
    ak_slab* const sU = (slab);            \
    sU->bk->fd = (sU->fd);                 \
    sU->fd->bk = (sU->bk);                 \
    sU->fd = sU->bk = AK_NULLPTR;          \
  } while (0)
struct ak_slab_tag
{
    ak_slab*      fd;
    ak_slab*      bk;
    ak_slab_root* root;
    ak_bitset512  avail;
    void*         _unused;
};
static void ak_slab_release_pages(ak_slab_root* root, ak_slab* s, ak_u32 numtofree)
{
    ak_slab* const r = s;
    ak_slab* next = AK_NULLPTR;
    s = s->fd;
    for (ak_u32 ct = 0; ct < numtofree; ++ct) {
        if (s == r) {
            break;
        } else {
            next = s->fd;
        }
        ak_slab_unlink(s);
        ak_os_free(s, AKMALLOC_DEFAULT_PAGE_SIZE);
        s = next;
    }
}
#define AK_NULLPTR NULL
#define ak_slab_link(slab, fwd, back)      \
  do {                                     \
    ak_slab* const sL = (slab);            \
    ak_slab* const fL = (fwd);             \
    ak_slab* const bL = (back);            \
    ak_slab_link_bk(sL, bL);               \
    ak_slab_link_fd(sL, fL);               \
  } while (0)

Context

StackExchange Code Review Q#135181, answer score: 5

Revisions (0)

No revisions yet.