1365194 - Build mozjemalloc as C++

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Description

[Mike Hommey [:glandium]]

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 1

[Mike Hommey [:glandium]]

Attachment: Bug 1365194 - Call moz_abort directly instead of using a macro to override abort.

Using -Dabort=moz_abort actually makes the build fail in some libstdc++
headers when building as C++.

Review commit: https://reviewboard.mozilla.org/r/139660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139660/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message.

The source file is renamed too, because the build system doesn't handle
sources changing suffix very well (at least not without a clobber).

Review commit: https://reviewboard.mozilla.org/r/139662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139662/

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8868059 [details]
Bug 1365194 - Call moz_abort directly instead of using a macro to override abort.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139660/diff/1-2/

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8868060 [details]
Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139662/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1365194 - Make `extern "C"` part of MOZ_MEMORY_API and MOZ_JEMALLOC_API.

This avoids many additions of `extern "C"` in C++ code and will avoid
having to do the same to mozjemalloc once built as C++.

Review commit: https://reviewboard.mozilla.org/r/139702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139702/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc.

The source file is renamed too, because the build system doesn't handle
sources changing suffix very well (at least not without a clobber).

The _GNU_SOURCE define is removed because GCC/Clang set it by default in
C++ mode.

Review commit: https://reviewboard.mozilla.org/r/139704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139704/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8868060 [details]
Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139662/diff/2-3/

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1365194 - Compile mozjemalloc as C++.

The source file is renamed too, because the build system doesn't handle
sources changing suffix very well (at least not without a clobber).

The _GNU_SOURCE define is removed because GCC/Clang set it by default in
C++ mode.

Review commit: https://reviewboard.mozilla.org/r/139708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139708/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8868087 [details]
Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139704/diff/1-2/

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8868087 [details]
Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139704/diff/2-3/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8868095 [details]
Bug 1365194 - Compile mozjemalloc as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139708/diff/1-2/

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8868095 [details]
Bug 1365194 - Compile mozjemalloc as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139708/diff/2-3/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8868095 [details]
Bug 1365194 - Compile mozjemalloc as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139708/diff/3-4/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8868095 [details]
Bug 1365194 - Compile mozjemalloc as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139708/diff/4-5/

Comment 15

[Nicholas Nethercote [inactive]]

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

Comment 16

[Nicholas Nethercote [inactive]]

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?

Comment 17

[Mike Hommey [:glandium]]

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

[Nicholas Nethercote [inactive]]

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?

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8868087 [details]
Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc.

https://reviewboard.mozilla.org/r/139704/#review143272

Comment 20

[Mike Hommey [:glandium]]

(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

[Nicholas Nethercote [inactive]]

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?

Comment 22

[Mike Hommey [:glandium]]

(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 23

[Mike Hommey [:glandium]]

Comment on attachment 8868059 [details]
Bug 1365194 - Call moz_abort directly instead of using a macro to override abort.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139660/diff/2-3/

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8868060 [details]
Bug 1365194 - Remove MOZ_JEMALLOC_API from _malloc_options and _malloc_message.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139662/diff/3-4/

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8868086 [details]
Bug 1365194 - Make `extern "C"` part of MOZ_MEMORY_API and MOZ_JEMALLOC_API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139702/diff/1-2/

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8868087 [details]
Bug 1365194 - Remove parts of the hacks for memalign in mozjemalloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139704/diff/3-4/

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8868095 [details]
Bug 1365194 - Compile mozjemalloc as C++.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139708/diff/5-6/

Comment 28

[Nicholas Nethercote [inactive]]

> 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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Mike Hommey [:glandium]]

(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

[Pulsebot]

Backout by [e-mail]:
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

[Carsten Book [:Tomcat]]

backed this out for breaking spidermonkey like https://treeherder.mozilla.org/logviewer.html#?job_id=99717207&repo=mozilla-central on windows

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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