Some mozjemalloc code cleanup

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(7 attachments)

Comment hidden (empty)
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)
(Assignee)

Updated

2 years ago
Blocks: 1365194
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
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 23

2 years ago
mozreview-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 24

2 years ago
mozreview-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 25

2 years ago
mozreview-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 26

2 years ago
mozreview-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+
(Assignee)

Comment 27

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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 29

2 years ago
mozreview-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+
(Assignee)

Comment 30

2 years ago
mozreview-review-reply
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.

Comment 32

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1365460
You need to log in before you can comment on or make changes to this bug.