[RDY] Use jemalloc instead of libc allocator #2415

tarruda · 2015-04-12T17:04:53Z

Add free wrapper to memory.c(xfree). Required for customizing the memory allocator.
Remove code that uses memory pools, use xmalloc/xfree instead. This simplifies code and memory caching can still be implemented in the allocator layer(see below)
Add option to use jemalloc instead of libc allocator

I'm doing this more in preparation for #2371 which will require thread safety when dealing with memory allocated for queued events(memory pools would need to be explicitly synchronized, increasing complexity even more), but it should also improve overall allocation/deallocation efficiency due to reduced fragmentation and jemalloc's thread cache for small objects.

Could possibly fix #650, (cc @aktau)

justinmk · 2015-04-12T18:32:37Z

src/nvim/memory.c

+/// free wrapper that returns delegates to the backing memory manager
+void xfree(void *ptr)
+{
+  free(ptr);


What's the purpose of xfree(), if we redefine free()? Is the plan to call je_* explicitly from the xfoo family, in the future?

In this module, free is only a placeholder for the actual free implementation(which is je_free when USE_JEMALLOC is defined). We still need a fixed function address for the other modules to link against. Redefining free as a macro globally is definitely not a good approach(we lose sanity when setting breakpoints, for example)

Is the plan to call je_* explicitly from the xfoo family, in the future?

No, but we could eventually use this as a slot for extra deallocation logic. For example, see how redis keeps track of how much memory is currently used in zmalloc.c.

We still need a fixed function address for the other modules to link against. Redefining free as a macro globally is definitely not a good approach(we lose sanity when setting breakpoints, for example)

That makes perfect sense, thanks.

This causes a "read after free" error when kmp_free is replaced by `free`.

Klib pools were used to improve allocation efficiency for some small objects, but it is not a thread-safe approach. Thread safety in allocations will be required for implementing neovim#2371).

tarruda · 2015-04-12T21:49:11Z

@jszakmeister any idea what is causing the quickbuild failure? It seems to be bumping into jemalloc Makefile errors

splinterofchaos · 2015-04-12T22:10:48Z

src/nvim/lib/klist.h

@@ -95,7 +95,7 @@
            kmp_free(name, kl->mp, p);                                  \
        kmp_free(name, kl->mp, p);                                      \
        kmp_destroy(name, kl->mp);                                      \
-        free(kl);                                                       \
+        xfree(kl);                                                       \


splinterofchaos · 2015-04-13T00:04:31Z

I'm doing this more in preparation for #2371 which will require thread safety when dealing with memory

I've been under the impression for a long time that GNU's malloc implementation is thread safe (when compiled with -pthreads), and some light googling suggests BSD uses jemalloc already, but is this not true for Mac or Windows?

Some reading suggests that jemalloc is very good, even if thread safety isn't the main advantage, although I haven't seen a comparison against other allocation strategies yet.

jszakmeister · 2015-04-13T00:05:47Z

third-party/cmake/BuildJeMalloc.cmake

+    -DTARGET=jemalloc
+    -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/DownloadAndExtractFile.cmake
+  BUILD_IN_SOURCE 1
+  CONFIGURE_COMMAND sh ${DEPS_BUILD_DIR}/src/jemalloc/autogen.sh &&


This is failing on FreeBSD because the default BUILD_COMMAND invokes make, which is BSD make on FreeBSD. The install should build everything required, so all you need to do here is add BUILD_COMMAND "". HTH!

👍 thanks

We already use wrappers for allocation, the new `xfree` function is the equivalent for deallocation and provides a way to fully replace the malloc implementation used by Neovim.

Jemalloc will be used if the cmake option `USE_JEMALLOC` is enabled(which is the default). To avoid trouble with clang's ASAN, it is disabled by default if the `SANITIZE` option is enabled. Since jemalloc has thread cache for small objects, it fills the gap created by removing klib memory pools. The `xstrdup` funciton(memory.c) had to be reimplemented on top of `xmalloc` to make it work with a custom allocator.

tarruda · 2015-04-13T11:33:49Z

I've been under the impression for a long time that GNU's malloc implementation is thread safe (when compiled with -pthreads), and some light googling suggests BSD uses jemalloc already, but is this not true for Mac or Windows?

What I meant is that we relied on klib memory pools to optimize some event data allocations, and this would require synchronization for #2371 since the event producer and consumer would be running in different threads.

The commit with replaces klib mempool by malloc/free takes care of the thread-safety problem, and the commit which adds jemalloc as a dependency fills the memory pool gap since it maintains thread-local cache for small allocations(it's also much simpler to leave caching to the allocator due to code simplification.

marvim added the RFC label Apr 12, 2015

tarruda force-pushed the use-jemalloc branch from 4e44976 to 188c79a Compare April 12, 2015 18:18

justinmk added the refactor changes that are not features or bugfixes label Apr 12, 2015

justinmk reviewed Apr 12, 2015
View reviewed changes

tarruda force-pushed the use-jemalloc branch 3 times, most recently from 96152be to 6988907 Compare April 12, 2015 20:13

tarruda added 2 commits April 12, 2015 17:20

eval: Fix memory deallocation of JobEvent

27b5ef3

This causes a "read after free" error when kmp_free is replaced by `free`.

memory: Replace klib memory pools by malloc/free

ba10e31

Klib pools were used to improve allocation efficiency for some small objects, but it is not a thread-safe approach. Thread safety in allocations will be required for implementing neovim#2371).

tarruda force-pushed the use-jemalloc branch 2 times, most recently from 03a23fb to dbebead Compare April 12, 2015 21:29

splinterofchaos reviewed Apr 12, 2015
View reviewed changes

jszakmeister reviewed Apr 13, 2015
View reviewed changes

tarruda added 3 commits April 13, 2015 08:22

memory: Add free wrapper and refactor project to use it

34c48aa

We already use wrappers for allocation, the new `xfree` function is the equivalent for deallocation and provides a way to fully replace the malloc implementation used by Neovim.

clint: Add rules to check for memory functions calls

2d104f1

tarruda force-pushed the use-jemalloc branch from dbebead to b40e08d Compare April 13, 2015 11:23

tarruda changed the title ~~[RFC] Use jemalloc instead of libc allocator~~ [RDY] Use jemalloc instead of libc allocator Apr 13, 2015

tarruda force-pushed the use-jemalloc branch from b40e08d to 2d104f1 Compare April 13, 2015 11:45

marvim added RDY and removed RFC labels Apr 13, 2015

tarruda merged commit 2d104f1 into neovim:master Apr 13, 2015

tarruda added a commit that referenced this pull request Apr 13, 2015

Merge PR #2415 'Use jemalloc instead of libc allocator'

a9ee85b

jszakmeister removed the RDY label Apr 13, 2015

tarruda mentioned this pull request Apr 13, 2015

Is the nfa regex engine slow? #650

Open

elmart mentioned this pull request Apr 13, 2015

18 new coverity issues #2422

Closed

justinmk mentioned this pull request Apr 24, 2015

[RDY] third-party dependencies for Windows #2495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] Use jemalloc instead of libc allocator #2415

[RDY] Use jemalloc instead of libc allocator #2415

tarruda commented Apr 12, 2015

justinmk Apr 12, 2015

tarruda Apr 12, 2015

justinmk Apr 12, 2015

tarruda commented Apr 12, 2015

splinterofchaos Apr 12, 2015

tarruda Apr 13, 2015

splinterofchaos commented Apr 13, 2015

jszakmeister Apr 13, 2015

tarruda Apr 13, 2015

tarruda commented Apr 13, 2015

[RDY] Use jemalloc instead of libc allocator #2415

[RDY] Use jemalloc instead of libc allocator #2415

Conversation

tarruda commented Apr 12, 2015

justinmk Apr 12, 2015

Choose a reason for hiding this comment

tarruda Apr 12, 2015

Choose a reason for hiding this comment

justinmk Apr 12, 2015

Choose a reason for hiding this comment

tarruda commented Apr 12, 2015

splinterofchaos Apr 12, 2015

Choose a reason for hiding this comment

tarruda Apr 13, 2015

Choose a reason for hiding this comment

splinterofchaos commented Apr 13, 2015

jszakmeister Apr 13, 2015

Choose a reason for hiding this comment

tarruda Apr 13, 2015

Choose a reason for hiding this comment

tarruda commented Apr 13, 2015