Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for jemalloc #6

Open
wants to merge 6 commits into
base: 8.1
Choose a base branch
from
Open

Add support for jemalloc #6

wants to merge 6 commits into from

Conversation

tojamrok
Copy link

@tojamrok tojamrok commented Feb 6, 2018

jemalloc handles threaded allocations better than the default glibc's ptmalloc2, but it's less relevant for Pike. Smaller memory fragmentation is more tangible.

You need jemalloc library available on your OS to be able to compile Pike with jemalloc. Build it with make CONFIGUREARGS="--enable-jemalloc".

The only externally visible change is in _memory_usage() function which depends on malloc's implementation-specific details. glibc's malloc provides struct mallinfo mallinfo(), while jemalloc provides int mallctl(const char *name, void *oldp, size_t *oldlenp, void *newp, size_t newlen); where name argument starts with "stats." (see man jemalloc).

Tomasz Jamroszczak added 6 commits February 6, 2018 13:10
It's not needed to use C99 compilation but jemalloc #includes <stdbool.h>.
builtin_functions.c uses non-standard dlmallinfo() or mallinfo().  These
utility functions are not implemented by jemalloc.  Instead it provides
malloc_stats_print() and mallctl().
Just add conditional include.  Depends on configure.in.
When configure.in sees --enable-jemalloc it does following:
1. adds -ljemalloc to LIBS and
2. adds USE_JEMALLOC C preprocessor define.
3. adds HAVE_JEMALLOC_JEMALLOC_H C preprocessor definition if the library is
present during autoconfigure.
4. Removes #include <malloc.h>.
Don't do fancy plumbering with missing mallinfo if USE_JEMALLOC is defined.
@PeterBortas
Copy link
Contributor

This has been sitting uncommented since forever. Minireview in case you still want this in:

  • There is unrelated fiddling with variable names in HTTPLoop (unless that's some sort of namespace pollution from the allocator)
  • It's rewriting macros in builtin.cmod just for the sake of it as far as I can see

I don't see why it couldn't be added at least for a while so it would be easier to benchmark between allocators, but the commit should be as minimal as possible.

@tojamrok
Copy link
Author

Regarding both comments about src/modules/HTTPLoop/requestobject.c and src/bultin.cmod: both changes are needed here because the code clashes with <stdbool.h>. It's funny that jemalloc includes <stdbool.h> even if it doesn't need C99 flavor to compile.

In <stdbool.h> there's #define true 1. Thus in src/modules/HTTPLoop/requestobject.c change from true to true_val is needed.

In src/builtin.cmod the GET_VAL(NAME) macro is called as GET_VAL (true) which translates to PIKE_CONCAT (get_val_, 1) inside the GET_VAL(NAME) macro which in turn translates to get_val_##1 but other code wants to call get_val_true. The same about TOSTR(true) which translates to #1 but we'd rather have #true instead.

What I'm more worried about is that the commit is too small. There are places where memory is allocated outside of malloc which could clash with jemalloc in unforeseen ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants