Closed Bug 559263 Opened 10 years ago Closed 9 years ago

make jemalloc work with android

Categories

(Core :: Memory Allocator, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: blassey, Assigned: blassey)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #438939 - Flags: review?(vladimir)
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?
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.
Depends on: 561130
with the patch on bug 561130 we can replace the allocator across the board and effectively eliminate the need to call isalloc()
Attached patch patch (obsolete) — Splinter Review
this is a roll up of all the jemalloc patches that have been committed to vlad's android repo
Attachment #438939 - Attachment is obsolete: true
Attachment #440958 - Flags: review?(vladimir)
Attachment #438939 - Flags: review?(vladimir)
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 on attachment 440958 [details] [diff] [review]
patch

(r- based on previous comments)
Attachment #440958 - Flags: review?(vladimir) → review-
Attached patch patchSplinter Review
Assignee: nobody → blassey.bugs
Attachment #440958 - Attachment is obsolete: true
Attachment #447276 - Flags: review?(vladimir)
Attached patch nspr patch (obsolete) — Splinter Review
Attachment #447277 - Flags: review?(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.
Attachment #447277 - Flags: review?(ted.mielczarek) → review-
Attachment #447277 - Attachment is obsolete: true
Attachment #448844 - Flags: review?(ted.mielczarek)
Attached patch follow up patch (obsolete) — Splinter Review
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.
Attachment #448849 - Flags: review?(ted.mielczarek)
Attached patch follow up patch (obsolete) — Splinter Review
Attachment #448862 - Flags: review?(ted.mielczarek)
Attachment #448849 - Attachment is obsolete: true
Attachment #448849 - Flags: review?(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
(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
Attachment #448844 - Flags: review?(ted.mielczarek) → review+
Attachment #448862 - Flags: review?(ted.mielczarek) → review+
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
Attached patch follow up patchSplinter Review
Attachment #448862 - Attachment is obsolete: true
Attachment #449317 - Flags: review?(ted.mielczarek)
Attachment #449317 - Flags: review?(ted.mielczarek) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/098cf0d0c599 and updated nspr http://hg.mozilla.org/mozilla-central/rev/b5b016bb7c91
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Version: unspecified → Trunk
Depends on: 826171
You need to log in before you can comment on or make changes to this bug.