559263 - make jemalloc work with android

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

One challenge for jemalloc on android is that as far as I can tell we cannot avoid having some memory allocated by the system since its allocated by the jvm. This patch does two things to deal with that. First it exposes sys_free and sys_delete functions to free memory we know jemalloc didn't allocate.  Second, jemalloc's free function checks that the memory was allocated by jemalloc before freeing it, otherwise it uses sys_free.

Comment 1

[Benjamin Smedberg]

That looks like a fairly expensive check. Have we measured to make sure that jemalloc is worth it compared with the system allocator with this patch?

Comment 2

[Stuart Parmenter]

checking for every free will be pretty expensive.  Do we know what things we're passing from java inwards?  I would hope we could stop those at the JNI boundaries.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

with the patch on bug 561130 we can replace the allocator across the board and effectively eliminate the need to call isalloc()

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this is a roll up of all the jemalloc patches that have been committed to vlad's android repo

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 440958 [details] [diff] [review]
patch

Looks fine, but for two things:

- make the same change to js/src/configure.in that was made to the toplevel configure.in, for setting MOZ_MEMORY_ANDROID (don't do the nested case etc.)

- you're going to have to split out the nspr patch and get review from ted on that; just include the configure.in change, not the configure diff, because it makes the patch look scarier than it is :)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 440958 [details] [diff] [review]
patch

(r- based on previous comments)

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: nspr patch

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/d3c103ccd5f9

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 447277 [details] [diff] [review]
nspr patch

>diff -r f897d00a23f5 -r bf8bd3aa831a nsprpub/configure.in
>--- a/nsprpub/configure.in	Mon May 24 21:18:39 2010 -0400
>+++ b/nsprpub/configure.in	Mon May 24 21:19:14 2010 -0400
>@@ -1556,7 +1556,7 @@
>     CXXFLAGS="$CXXFLAGS -Wall"
>     MDCPUCFG_H=_linux.cfg
>     PR_MD_CSRCS=linux.c
>-    MKSHLIB='$(CC) $(DSO_LDOPTS) -o $@'
>+    MKSHLIB='$(CC) $(DSO_LDOPTS) $(WRAP_MALLOC_CFLAGS) $(WRAP_MALLOC_LIB) -o $@'

You include WRAP_MALLOC_CFLAGS in $(DSO_LDOPTS) in the next hunk of your patch, so won't this double-include it?

> dnl ========================================================
>+dnl = Use malloc wrapper lib
>+dnl ========================================================
>+AC_ARG_ENABLE(wrap-malloc,
>+[  --enable-wrap-malloc    Wrap malloc calls (gnu linker only)],
>+[     if test "$enableval" = "yes"; then
>+	    _WRAP_MALLOC=1
>+      fi ])
>+
>+if test -n "$_WRAP_MALLOC"; then
>+    if test "$GNU_CC"; then

I think you want to test GCC_USE_GNU_LD.

>+       WRAP_MALLOC_CFLAGS="${LDFLAGS} -Wl,--wrap -Wl,malloc -Wl,--wrap -Wl,calloc -Wl,--wrap -Wl,valloc -Wl,--wrap -Wl,free -Wl,--wrap -Wl,realloc -Wl,--wrap -Wl,memalign -Wl,--wrap -Wl,__builtin_new -Wl,--wrap -Wl,__builtin_vec_new -Wl,--wrap -Wl,__builtin_delete -Wl,--wrap -Wl,__builtin_vec_delete -Wl,--wrap -Wl,PR_Free -Wl,--wrap -Wl,PR_Malloc -Wl,--wrap -Wl,PR_Calloc -Wl,--wrap -Wl,PR_Realloc -Wl,--wrap -Wl,strdup -Wl,--wrap -Wl,strndup"
>+       DSO_LDOPTS="$DSO_LDOPTS $WRAP_MALLOC_CFLAGS"
>+    fi

Since this is GNU LD only, you should AC_MESSAGE_ERROR if it's specified and we're not using GNU LD.

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: nspr patch, nits addressed

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

this addresses cjones's comments in bug 561130 about wrapping posix-memalign and making --enable-jemalloc on android imply --enable-wrap-malloc and --width-wrap-malloc=-lmozalloc

also, I noticed that the --enable-thumb2 subconfigure argument was only being passed to nspr (not js) so I fixed that by moving it up.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to comment #12)
> also, I noticed that the --enable-thumb2 subconfigure argument was only being
> passed to nspr (not js) so I fixed that by moving it up.

FYI, I asked you to fix this in bug 563751 comment 18. If you're going to ignore my r+ with comments, I'm going to start r-ing you if I have any comments. :-P

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #14)
> (In reply to comment #12)
> > also, I noticed that the --enable-thumb2 subconfigure argument was only being
> > passed to nspr (not js) so I fixed that by moving it up.
> 
> FYI, I asked you to fix this in bug 563751 comment 18. If you're going to
> ignore my r+ with comments, I'm going to start r-ing you if I have any
> comments. :-P

I misunderstood your comment and put a sub-configure clause in js/src/configure.in:
http://hg.mozilla.org/mozilla-central/rev/c3af7e83e26f#l1.60

That can probably be removed I'd assume

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 448844 [details] [diff] [review]
nspr patch, nits addressed

Committed to NSPR CVS:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.277; previous revision: 1.276
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.281; previous revision: 1.280
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.41; previous revision: 1.40
done

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: follow up patch

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/098cf0d0c599 and updated nspr http://hg.mozilla.org/mozilla-central/rev/b5b016bb7c91