Closed Bug 680373 Opened 9 years ago Closed 9 years ago

Link jemalloc to mozutils instead of mozalloc on Android

Categories

(Core :: Memory Allocator, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 file, 5 obsolete files)

Currently, jemalloc is integrated by using libmozalloc to embed it, which ends up being different from other platforms, and also makes bug 677501 harder.
Attachment #554336 - Flags: review?(blassey.bugs)
Attachment #554367 - Flags: review?(blassey.bugs)
Attachment #554336 - Attachment is obsolete: true
Attachment #554336 - Flags: review?(blassey.bugs)
This should be the one, hopefully. It also skips building the fennec binary, which we don't use anyways.
Attachment #554413 - Flags: review?(blassey.bugs)
Attachment #554367 - Attachment is obsolete: true
Attachment #554367 - Flags: review?(blassey.bugs)
Assignee: nobody → mh+mozilla
Blocks: 680440
This one only adds an ifdef MOZ_MEMORY in other-licences/android/Makefile.in
Attachment #554430 - Flags: review?(blassey.bugs)
Attachment #554413 - Attachment is obsolete: true
Attachment #554413 - Flags: review?(blassey.bugs)
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
Attachment #554413 - Attachment is obsolete: false
Attachment #554413 - Attachment is obsolete: true
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__.
Attachment #554450 - Flags: review?(blassey.bugs)
Attachment #554430 - Attachment is obsolete: true
Attachment #554430 - Flags: review?(blassey.bugs)
Attachment #554450 - Flags: review?(blassey.bugs) → review+
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.
Attachment #554886 - Flags: review?(pbiggar)
Attachment #554450 - Attachment is obsolete: true
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".
Attachment #554886 - Flags: review?(pbiggar) → review+
(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.
(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.
(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!
Blocks: 681588
http://hg.mozilla.org/mozilla-central/rev/2921dbf1def9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.