Closed Bug 1365194 Opened 7 years ago Closed 7 years ago

Build mozjemalloc as C++

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(5 files)

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.
Attachment #8868059 - Flags: review?(n.nethercote)
Attachment #8868060 - Flags: review?(n.nethercote)
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 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+
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 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 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+
(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 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+
(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.
> 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.
Blocks: 1365460
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
(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.
Blocks: 1365473
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
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)
Depends on: 1365788
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
Flags: needinfo?(mh+mozilla)
Depends on: 1395063
You need to log in before you can comment on or make changes to this bug.