Closed
Bug 1365194
Opened 7 years ago
Closed 7 years ago
Build mozjemalloc as C++
Categories
(Core :: Memory Allocator, enhancement)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
n.nethercote
:
review+
|
Details |
As per https://groups.google.com/d/msg/mozilla.dev.platform/S8WA3oHB7mM/4BsGua72AQAJ the expected benefits are RAII (for e.g. locks) and templates to replace some of the gory macros. Obviously, building as C++ is not going to do that magically for us, but it's the first step to be able to.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8868059 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•7 years ago
|
Attachment #8868060 -
Flags: review?(n.nethercote)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8868059 [details] Bug 1365194 - Call moz_abort directly instead of using a macro to override abort. https://reviewboard.mozilla.org/r/139660/#review143262
Attachment #8868059 -
Flags: review?(n.nethercote) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8868060 [details] Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message. https://reviewboard.mozilla.org/r/139662/#review143264 ::: commit-message-b82d8:19 (Diff revision 3) > +They also allow run-time override through LD_PRELOAD, but one might as > +well use the MALLOC_OPTIONS environment variable for _malloc_options. > +As for _malloc_message, it doesn't seem very useful to override, and > +probably noone ever overrode it at runtime. > + > +Note, we may want to remove them in a followup. By "them" do you mean _malloc_options and _malloc_message?
Attachment #8868060 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868060 [details] Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message. https://reviewboard.mozilla.org/r/139662/#review143264 > By "them" do you mean _malloc_options and _malloc_message? Yes.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8868086 [details] Bug 1365194 - Make `extern "C"` part of MOZ_MEMORY_API and MOZ_JEMALLOC_API. https://reviewboard.mozilla.org/r/139702/#review143270 ::: memory/build/mozmemory_wrap.h:125 (Diff revision 1) > > #include "mozilla/Types.h" > > +#ifndef MOZ_EXTERN_C > +#ifdef __cplusplus > +#define MOZ_EXTERN_C extern "C" Would this go better in mfbt/Types.h, next to MOZ_{BEGIN,END}_EXTERN_C?
Attachment #8868086 -
Flags: review?(n.nethercote) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8868087 [details] Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc. https://reviewboard.mozilla.org/r/139704/#review143272
Attachment #8868087 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #18) > > +#ifndef MOZ_EXTERN_C > > +#ifdef __cplusplus > > +#define MOZ_EXTERN_C extern "C" > > Would this go better in mfbt/Types.h, next to MOZ_{BEGIN,END}_EXTERN_C? I'm not sure. Things that need an extern "C" can use MOZ_{BEGIN,END}_EXTERN_C. I don't think we have other use cases for macros implying an extern "C" as part of their expansion (essentially the MOZ_JEMALLOC_API and MOZ_MEMORY_API macros are the only one I can see to ever need this)
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8868095 [details] Bug 1365194 - Compile mozjemalloc as C++. https://reviewboard.mozilla.org/r/139708/#review143274 Is it worth renaming jemalloc_types.h as mozjemalloc_types.h for consistency? ::: memory/mozjemalloc/moz.build:27 (Diff revision 5) > > LOCAL_INCLUDES += [ > '/memory/build', > ] > > if CONFIG['GNU_CC']: Should this now check GNU_CXX instead of GNU_CC?
Attachment #8868095 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #21) > Comment on attachment 8868095 [details] > Bug 1365194 - Compile mozjemalloc as C++. > > https://reviewboard.mozilla.org/r/139708/#review143274 > > Is it worth renaming jemalloc_types.h as mozjemalloc_types.h for consistency? The same argument could be made about all the jemalloc_ APIs, might as well do all of that in one batch. In a followup? > ::: memory/mozjemalloc/moz.build:27 > (Diff revision 5) > > > > LOCAL_INCLUDES += [ > > '/memory/build', > > ] > > > > if CONFIG['GNU_CC']: > > Should this now check GNU_CXX instead of GNU_CC? Fair enough.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
> The same argument could be made about all the jemalloc_ APIs, might as well do all of that in one batch. In a followup?
I suggest renaming jemalloc_types.h in this patch for filename consistency. I don't think the APIs need renaming, but if you think it's worthwhile, a follow-up is fine.
Comment 29•7 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/484c5137e19f Call moz_abort directly instead of using a macro to override abort. r=njn https://hg.mozilla.org/integration/autoland/rev/d8de424663b7 Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message. r=njn https://hg.mozilla.org/integration/autoland/rev/b56224bf370d Make `extern "C"` part of MOZ_MEMORY_API and MOZ_JEMALLOC_API. r=njn https://hg.mozilla.org/integration/autoland/rev/4b9c2fb40653 Remove parts of the hacks for memalign in mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/9711f5bbda3b Compile mozjemalloc as C++. r=njn
Assignee | ||
Comment 30•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #21) > Comment on attachment 8868095 [details] > Bug 1365194 - Compile mozjemalloc as C++. > > https://reviewboard.mozilla.org/r/139708/#review143274 > > Is it worth renaming jemalloc_types.h as mozjemalloc_types.h for consistency? Damn, I had it amended locally, and had a mozreview push prompt to publish it, but forgot to accept before autolanding... will file a followup.
Comment 31•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/ecd952de286e Backed out changeset 9711f5bbda3b https://hg.mozilla.org/mozilla-central/rev/7ed15a5bad2f Backed out changeset 4b9c2fb40653 https://hg.mozilla.org/mozilla-central/rev/c361b0453daa Backed out changeset b56224bf370d https://hg.mozilla.org/mozilla-central/rev/056723013e36 Backed out changeset d8de424663b7 https://hg.mozilla.org/mozilla-central/rev/6e3ca5b38f71 Backed out changeset 484c5137e19f to clean up spidermonkey
Comment 32•7 years ago
|
||
backed this out for breaking spidermonkey like https://treeherder.mozilla.org/logviewer.html#?job_id=99717207&repo=mozilla-central on windows
Flags: needinfo?(mh+mozilla)
Comment 33•7 years ago
|
||
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/20616a874799 Call moz_abort directly instead of using a macro to override abort. r=njn https://hg.mozilla.org/integration/autoland/rev/75dad4ff5874 Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message. r=njn https://hg.mozilla.org/integration/autoland/rev/3284fc12e07d Make `extern "C"` part of MOZ_MEMORY_API and MOZ_JEMALLOC_API. r=njn https://hg.mozilla.org/integration/autoland/rev/5d681afa6b7a Remove parts of the hacks for memalign in mozjemalloc. r=njn https://hg.mozilla.org/integration/autoland/rev/76a51cf81ac8 Compile mozjemalloc as C++. r=njn
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20616a874799 https://hg.mozilla.org/mozilla-central/rev/75dad4ff5874 https://hg.mozilla.org/mozilla-central/rev/3284fc12e07d https://hg.mozilla.org/mozilla-central/rev/5d681afa6b7a https://hg.mozilla.org/mozilla-central/rev/76a51cf81ac8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(mh+mozilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•