Closed
Bug 680373
Opened 13 years ago
Closed 13 years ago
Link jemalloc to mozutils instead of mozalloc on Android
Categories
(Core :: Memory Allocator, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: glandium, Assigned: glandium)
References
Details
(Whiteboard: fixed-in-bs)
Attachments
(1 file, 5 obsolete files)
17.60 KB,
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Attachment #554336 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #554367 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•13 years ago
|
Attachment #554336 -
Attachment is obsolete: true
Attachment #554336 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
Attachment #554367 -
Attachment is obsolete: true
Attachment #554367 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mh+mozilla
Assignee | ||
Comment 4•13 years ago
|
||
This one only adds an ifdef MOZ_MEMORY in other-licences/android/Makefile.in
Attachment #554430 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•13 years ago
|
Attachment #554413 -
Attachment is obsolete: true
Attachment #554413 -
Flags: review?(blassey.bugs)
Comment 5•13 years ago
|
||
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•13 years ago
|
Attachment #554413 -
Attachment is obsolete: true
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
Attachment #554430 -
Attachment is obsolete: true
Attachment #554430 -
Flags: review?(blassey.bugs)
Updated•13 years ago
|
Attachment #554450 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
Attachment #554450 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 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•13 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•13 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•13 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•13 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 | ||
Comment 13•13 years ago
|
||
http://hg.mozilla.org/projects/build-system/rev/2921dbf1def9
Whiteboard: fixed-in-bs
Assignee | ||
Comment 14•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2921dbf1def9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•