patterncMinor
New malloc implementation
Viewed 0 times
mallocimplementationnew
Problem
I've written a new
The goals for this library are:
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 =
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
rather than
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.
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
I might be able to figure out what
mean (avail and _unused are pretty clear), but I have no clue
what
the same problem with
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
Better yet, I would just use NULL.
Macro Nesting Duplicates Code
If I act as the C PreProcessor what I get for this macro
is the following:
When it could simply be:
Please note that the only place
and
The
functions.
If I needed to maintain
implementation it is quite confusing.
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 aprefix 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 _unusedmean (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 havethe 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 NULLBetter 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 severalfunctions.
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.