Closed Bug 1365191 Opened 4 years ago Closed 4 years ago

Some mozjemalloc code cleanup

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

(7 files)

No description provided.
Blocks: 1365194
Comment on attachment 8868048 [details]
Bug 1365191 - Remove #if 0 sections in mozjemalloc.

https://reviewboard.mozilla.org/r/139644/#review143246
Attachment #8868048 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868049 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868050 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868051 [details]
Bug 1365191 - Remove MALLOC_UTRACE from mozjemalloc.

https://reviewboard.mozilla.org/r/139650/#review143252
Attachment #8868051 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868052 [details]
Bug 1365191 - Remove MALLOC_STATS from mozjemalloc.

https://reviewboard.mozilla.org/r/139652/#review143254
Attachment #8868052 - Flags: review?(n.nethercote) → review+
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 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?
Attachment #8868053 - Flags: review?(n.nethercote) → review+
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
Attachment #8868054 - Flags: review?(n.nethercote) → review+
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...
> 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.
Pushed by mh@glandium.org:
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
Blocks: 1365460
You need to log in before you can comment on or make changes to this bug.