1365191 - Some mozjemalloc code cleanup

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

Comment 1

[Mike Hommey [:glandium]]

Created attachment 8868048 [details]
Bug 1365191 - Remove #if 0 sections in mozjemalloc.

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

Comment 2

[Mike Hommey [:glandium]]

Created attachment 8868049 [details]
Bug 1365191 - Remove !MOZ_MEMORY sections in mozjemalloc.

MOZ_MEMORY is always defined when building mozjemalloc. Due to the
origin of the code, this was all FreeBSD-specific code, and if we want
to add FreeBSD support, we will probably need to add some of it, but I'd
rather avoid keeping the difference between FreeBSD and other posix
systems if we can.

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

Comment 3

[Mike Hommey [:glandium]]

Created attachment 8868050 [details]
Bug 1365191 - Remove MALLOC_VALIDATE in mozjemalloc.

It's always set.

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

Comment 4

[Mike Hommey [:glandium]]

Created attachment 8868051 [details]
Bug 1365191 - Remove MALLOC_UTRACE from mozjemalloc.

It's always unset, and Firefox has the logalloc replace-malloc library
for something similar.

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

Comment 5

[Mike Hommey [:glandium]]

Created attachment 8868052 [details]
Bug 1365191 - Remove MALLOC_STATS from mozjemalloc.

It's always set.

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

Comment 6

[Mike Hommey [:glandium]]

Created attachment 8868053 [details]
Bug 1365191 - Remove MALLOC_FILL from mozjemalloc.

It's always set.

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

Comment 7

[Mike Hommey [:glandium]]

Created attachment 8868054 [details]
Bug 1365191 - Remove dead code hidden behind the never set NEEDS_PTHREAD_MMAP_UNALIGNED_TSD.

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

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8868048 [details]
Bug 1365191 - Remove #if 0 sections in mozjemalloc.

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

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8868049 [details]
Bug 1365191 - Remove !MOZ_MEMORY sections in mozjemalloc.

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

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8868050 [details]
Bug 1365191 - Remove MALLOC_VALIDATE in mozjemalloc.

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

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8868051 [details]
Bug 1365191 - Remove MALLOC_UTRACE from mozjemalloc.

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

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8868052 [details]
Bug 1365191 - Remove MALLOC_STATS from mozjemalloc.

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

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8868053 [details]
Bug 1365191 - Remove MALLOC_FILL from mozjemalloc.

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

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8868054 [details]
Bug 1365191 - Remove dead code hidden behind the never set NEEDS_PTHREAD_MMAP_UNALIGNED_TSD.

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

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8868048 [details]
Bug 1365191 - Remove #if 0 sections in mozjemalloc.

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

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8868049 [details]
Bug 1365191 - Remove !MOZ_MEMORY sections in mozjemalloc.

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

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8868050 [details]
Bug 1365191 - Remove MALLOC_VALIDATE in mozjemalloc.

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

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8868051 [details]
Bug 1365191 - Remove MALLOC_UTRACE from mozjemalloc.

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

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8868052 [details]
Bug 1365191 - Remove MALLOC_STATS from mozjemalloc.

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

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8868053 [details]
Bug 1365191 - Remove MALLOC_FILL from mozjemalloc.

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

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8868054 [details]
Bug 1365191 - Remove dead code hidden behind the never set NEEDS_PTHREAD_MMAP_UNALIGNED_TSD.

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

Comment 22

[Nicholas Nethercote [:njn]]

Comment on attachment 8868048 [details]
Bug 1365191 - Remove #if 0 sections in mozjemalloc.

https://reviewboard.mozilla.org/r/139644/#review143246

Comment 23

[Nicholas Nethercote [:njn]]

Comment on attachment 8868049 [details]
Bug 1365191 - Remove !MOZ_MEMORY sections in mozjemalloc.

https://reviewboard.mozilla.org/r/139646/#review143248

::: memory/mozjemalloc/jemalloc.c
(Diff revision 3)
>  #include <limits.h>
>  #ifndef SIZE_T_MAX
>  #  define SIZE_T_MAX	SIZE_MAX
>  #endif
>  #include <pthread.h>
> -#ifdef MOZ_MEMORY_DARWIN

This change seems out of place in this patch.

Comment 24

[Nicholas Nethercote [:njn]]

Comment on attachment 8868050 [details]
Bug 1365191 - Remove MALLOC_VALIDATE in mozjemalloc.

https://reviewboard.mozilla.org/r/139648/#review143250

I feel slightly bad that we're losing documentation about which parts of the code are just for validation, but if we always validate that feeling is probably not helpful.

Comment 25

[Nicholas Nethercote [:njn]]

Comment on attachment 8868051 [details]
Bug 1365191 - Remove MALLOC_UTRACE from mozjemalloc.

https://reviewboard.mozilla.org/r/139650/#review143252

Comment 26

[Nicholas Nethercote [:njn]]

Comment on attachment 8868052 [details]
Bug 1365191 - Remove MALLOC_STATS from mozjemalloc.

https://reviewboard.mozilla.org/r/139652/#review143254

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8868049 [details]
Bug 1365191 - Remove !MOZ_MEMORY sections in mozjemalloc.

https://reviewboard.mozilla.org/r/139646/#review143248

> This change seems out of place in this patch.

This changes comes from the fact that the last uses of those macros are removed in this patch.

Comment 28

[Nicholas Nethercote [:njn]]

Comment on attachment 8868053 [details]
Bug 1365191 - Remove MALLOC_FILL from mozjemalloc.

https://reviewboard.mozilla.org/r/139654/#review143258

opt_zero is always false and opt_poison is always true. Is it worth getting rid of those constants?

Comment 29

[Nicholas Nethercote [:njn]]

Comment on attachment 8868054 [details]
Bug 1365191 - Remove dead code hidden behind the never set NEEDS_PTHREAD_MMAP_UNALIGNED_TSD.

https://reviewboard.mozilla.org/r/139656/#review143260

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8868053 [details]
Bug 1365191 - Remove MALLOC_FILL from mozjemalloc.

https://reviewboard.mozilla.org/r/139654/#review143258

They can both be overridden at runtime with MALLOC_OPTIONS in debug builds. I'm not sure it's something worth keeping or not. I don't think there's much value in disabling poison, but enabling zero at runtime may or may not be useful.
Arguably, we could have a replace-malloc lib to provide that...

Comment 31

[Nicholas Nethercote [:njn]]

> I don't think there's much value in disabling poison, but enabling zero at runtime may or may not be useful.

I agree that the case for removing opt_poison is stronger than the case for removing opt_zero. I can't imagine ever wanting to disable poisoning.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2ac9d5ebe2f
Remove #if 0 sections in mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/03aeae77fd57
Remove !MOZ_MEMORY sections in mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/a33773e54f5d
Remove MALLOC_VALIDATE in mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/0f4289fed78b
Remove MALLOC_UTRACE from mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/e246baa0d780
Remove MALLOC_STATS from mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/09b32bedead3
Remove MALLOC_FILL from mozjemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/a0c7189d60e3
Remove dead code hidden behind the never set NEEDS_PTHREAD_MMAP_UNALIGNED_TSD. r=njn

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2ac9d5ebe2f
https://hg.mozilla.org/mozilla-central/rev/03aeae77fd57
https://hg.mozilla.org/mozilla-central/rev/a33773e54f5d
https://hg.mozilla.org/mozilla-central/rev/0f4289fed78b
https://hg.mozilla.org/mozilla-central/rev/e246baa0d780
https://hg.mozilla.org/mozilla-central/rev/09b32bedead3
https://hg.mozilla.org/mozilla-central/rev/a0c7189d60e3