Link jemalloc to mozutils instead of mozalloc on Android

RESOLVED FIXED in mozilla9

Status

()

Core
Memory Allocator
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla9
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
Currently, jemalloc is integrated by using libmozalloc to embed it, which ends up being different from other platforms, and also makes bug 677501 harder.
(Assignee)

Comment 1

6 years ago
Created attachment 554336 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android
Attachment #554336 - Flags: review?(blassey.bugs)
(Assignee)

Comment 2

6 years ago
Created attachment 554367 [details] [diff] [review]
Link jemalloc into mozutils instead of mozalloc on Android
Attachment #554367 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #554336 - Attachment is obsolete: true
Attachment #554336 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

6 years ago
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.
Attachment #554413 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #554367 - Attachment is obsolete: true
Attachment #554367 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Updated

6 years ago
Blocks: 680440
(Assignee)

Comment 4

6 years ago
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
Attachment #554430 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #554413 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
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__.
Attachment #554450 - Flags: review?(blassey.bugs)
(Assignee)

Updated

6 years ago
Attachment #554430 - Attachment is obsolete: true
Attachment #554430 - Flags: review?(blassey.bugs)
Attachment #554450 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 7

6 years ago
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.
Attachment #554886 - Flags: review?(pbiggar)
(Assignee)

Updated

6 years ago
Attachment #554450 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
The interdiff from bugzilla, for once, works well:
https://bugzilla.mozilla.org/attachment.cgi?oldid=554450&action=interdiff&newid=554886&headers=1

Comment 9

6 years ago
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+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
(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

6 years ago
(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!
(Assignee)

Updated

6 years ago
Blocks: 681588
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/projects/build-system/rev/2921dbf1def9
Whiteboard: fixed-in-bs
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/mozilla-central/rev/2921dbf1def9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Blocks: 696455
You need to log in before you can comment on or make changes to this bug.