Last Comment Bug 680373 - Link jemalloc to mozutils instead of mozalloc on Android
: Link jemalloc to mozutils instead of mozalloc on Android
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Mike Hommey [:glandium]
:
: Mike Hommey [:glandium]
Mentors:
Depends on: 678195
Blocks: 677501 680440 681588 696455
  Show dependency treegraph
 
Reported: 2011-08-19 01:29 PDT by Mike Hommey [:glandium]
Modified: 2011-10-21 12:24 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Link jemalloc into mozutils instead of mozalloc on Android (12.62 KB, patch)
2011-08-19 01:46 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Link jemalloc into mozutils instead of mozalloc on Android (13.16 KB, patch)
2011-08-19 04:28 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Link jemalloc into mozutils instead of mozalloc on Android (15.86 KB, patch)
2011-08-19 07:39 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Link jemalloc into mozutils instead of mozalloc on Android (15.88 KB, patch)
2011-08-19 08:45 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Link jemalloc into mozutils instead of mozalloc on Android (16.97 KB, patch)
2011-08-19 09:51 PDT, Mike Hommey [:glandium]
blassey.bugs: review+
Details | Diff | Splinter Review
Link jemalloc into mozutils instead of mozalloc on Android (17.60 KB, patch)
2011-08-22 09:49 PDT, Mike Hommey [:glandium]
paul.biggar: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-08-19 01:29:20 PDT
Currently, jemalloc is integrated by using libmozalloc to embed it, which ends up being different from other platforms, and also makes bug 677501 harder.
Comment 1 Mike Hommey [:glandium] 2011-08-19 01:46:46 PDT
Created attachment 554336 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android
Comment 2 Mike Hommey [:glandium] 2011-08-19 04:28:15 PDT
Created attachment 554367 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android
Comment 3 Mike Hommey [:glandium] 2011-08-19 07:39:27 PDT
Created attachment 554413 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

This should be the one, hopefully. It also skips building the fennec binary, which we don't use anyways.
Comment 4 Mike Hommey [:glandium] 2011-08-19 08:45:48 PDT
Created attachment 554430 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

This one only adds an ifdef MOZ_MEMORY in other-licences/android/Makefile.in
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2011-08-19 09:27:35 PDT
Comment on attachment 554413 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

Review of attachment 554413 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/jemalloc/jemalloc.c
@@ +5861,5 @@
> +#endif
> +
> +#define _concat(a, b) a ## _ ## b
> +#define __concat(a, b) _concat(a, b)
> +#define wrap(a) __concat(_wrapper, a)

can you comment on why this mess is needed?

seems like you could just have #define wrap(a) wrap_ ## a in the MOZ_MEMORY_ANDROID clause and #define wrap(a) je ## a in the else
Comment 6 Mike Hommey [:glandium] 2011-08-19 09:51:46 PDT
Created attachment 554450 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

This one gets entirely rid of the concat mess, as you suggested.
It also gets rid of the WRAP_MALLOC define, which was only remaining in a ifdef __GLIBC__ section, and android doesn't define __GLIBC__.
Comment 7 Mike Hommey [:glandium] 2011-08-22 09:49:28 PDT
Created attachment 554886 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

Most of the patch has been reviewed by blassey already. The only changes in this iteration are the wrapping macro definitions to avoid a failure on mac and a redefinition of the memalign alias so that it is caught by the wrapping macros.
Comment 8 Mike Hommey [:glandium] 2011-08-22 09:50:39 PDT
The interdiff from bugzilla, for once, works well:
https://bugzilla.mozilla.org/attachment.cgi?oldid=554450&action=interdiff&newid=554886&headers=1
Comment 9 Paul Biggar 2011-08-23 12:56:16 PDT
Comment on attachment 554886 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android

Review of attachment 554886 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, I really like the simplification this patch has brought it :)

::: memory/jemalloc/jemalloc.c
@@ +5857,5 @@
> +__wrap_PR_Free(void *ptr) __attribute__((alias("__wrap_free")));
> +
> +#else
> +#define wrap(a) je_ ## a
> +#endif

Why do you use a different prefix on android. It seems like you could use the same for both?

@@ +5877,5 @@
> +strndup(const char *src, size_t len) {
> +	char* dst = (char*) malloc(len + 1);
> +	if (dst)
> +		strncpy(dst, src, len + 1);
> +	return dst;

Id prefer if you kept the names as je_strndup and je_strdup. I know the preprocessor does this, but it's purpose is clearer the other way.

@@ +6033,5 @@
>  }
>  
>  #ifdef MOZ_MEMORY_ELF
> +extern void *
> +memalign(size_t alignment, size_t size) __attribute__((alias ("memalign_internal"), visibility ("default")));

This looks like it does the same thing, right?

I know this is a bit annoying, but could you do a tryserver build on Mac with MOZ_MEMORY enabled? I'd be a lot more confident of these changes if we saw it working (no need to look at performance or anything, just that the build and tests succeed).

::: memory/mozalloc/mozalloc.cpp
@@ -85,5 @@
>  #define calloc(a, b)            je_calloc(a, b)
> -#ifndef MOZ_MEMORY_DARWIN
> -   // These functions could be passed a memory region that was not allocated by
> -   // jemalloc, so use the system-provided functions, which will in turn call
> -   // the jemalloc versions when appropriate.

Please preserve this comment, though obviosuly the wording will have to change. Maybe: "we omit functions which could be passed a memory region that was not allocated by jemalloc (realloc, free and malloc_usable_size). Instead we use the system-provided functions, which will in turn call the jemalloc versions when appropriate".
Comment 10 Mike Hommey [:glandium] 2011-08-23 13:26:38 PDT
(In reply to Paul Biggar from comment #9)
> Comment on attachment 554886 [details] [diff] [review]
> Link jemalloc into mozutils instead of mozalloc on Android
> 
> Review of attachment 554886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, I really like the simplification this patch has brought it :)
> 
> ::: memory/jemalloc/jemalloc.c
> @@ +5857,5 @@
> > +__wrap_PR_Free(void *ptr) __attribute__((alias("__wrap_free")));
> > +
> > +#else
> > +#define wrap(a) je_ ## a
> > +#endif
> 
> Why do you use a different prefix on android. It seems like you could use
> the same for both?

I wanted to keep the je_* names on windows and mac, but android needs __wrap_* names.

> @@ +5877,5 @@
> > +strndup(const char *src, size_t len) {
> > +	char* dst = (char*) malloc(len + 1);
> > +	if (dst)
> > +		strncpy(dst, src, len + 1);
> > +	return dst;
> 
> Id prefer if you kept the names as je_strndup and je_strdup. I know the
> preprocessor does this, but it's purpose is clearer the other way.

How about an explicit wrap(strdup) and wrap(strndup), then?

> @@ +6033,5 @@
> >  }
> >  
> >  #ifdef MOZ_MEMORY_ELF
> > +extern void *
> > +memalign(size_t alignment, size_t size) __attribute__((alias ("memalign_internal"), visibility ("default")));
> 
> This looks like it does the same thing, right?

It does the same thing, except that as the macro is memalign(a, b), memalign is only replaced if there are arguments. We could use wrap(memalign) here too, though.

> I know this is a bit annoying, but could you do a tryserver build on Mac
> with MOZ_MEMORY enabled? I'd be a lot more confident of these changes if we
> saw it working (no need to look at performance or anything, just that the
> build and tests succeed).

I did, actually. Though I will definitely try again because I want to double check my last patch queue.
Comment 11 Mike Hommey [:glandium] 2011-08-23 13:27:53 PDT
(In reply to Mike Hommey [:glandium] from comment #10)
> I wanted to keep the je_* names on windows and mac, but android needs
> __wrap_* names.

And it does so because that's the symbols that ld's --wrap argument will be looking for.
Comment 12 Paul Biggar 2011-08-23 13:55:58 PDT
(In reply to Mike Hommey [:glandium] from comment #10)

> but android needs
> __wrap_* names.

Can you add a comment saying this?


> > @@ +5877,5 @@ 
> How about an explicit wrap(strdup) and wrap(strndup), then?

Yeah, sounds good.


> > @@ +6033,5 @@
> > I know this is a bit annoying, but could you do a tryserver build on Mac
> > with MOZ_MEMORY enabled? I'd be a lot more confident of these changes if we
> > saw it working (no need to look at performance or anything, just that the
> > build and tests succeed).
> 
> I did, actually. Though I will definitely try again because I want to double
> check my last patch queue.

Great!
Comment 13 Mike Hommey [:glandium] 2011-08-24 03:56:40 PDT
http://hg.mozilla.org/projects/build-system/rev/2921dbf1def9
Comment 14 Mike Hommey [:glandium] 2011-08-25 01:51:43 PDT
http://hg.mozilla.org/mozilla-central/rev/2921dbf1def9

Note You need to log in before you can comment on or make changes to this bug.