Some ideas from bug 1365191 and bug 1365194: - Remove opt_poison - Remove _malloc_options and _malloc_message Other ideas: - Remove unused macros from e.g. rb.h More ideas?

- Remove unused headers: qr.h and ql.h

- Remove MOZ_MEMORY_DEBUG? It's just a synonym for MOZ_DEBUG, I think? - Remove MALLOC_PRODUCTION, maybe? It's just equivalent to !MOZ_MEMORY_DEBUG, I think? - Remove MOZ_WIDGET_GONK stuff (there's not much of it). - MALLOC_SYSV is only defined if MALLOC_PRODUCTION is undefined. That seems really surprising -- surely we want the same semantics always? And can opt_sysv be removed as well? - Remove MALLOC_BALANCE? - Remove JEMALLOC_MUNMAP/config_munmap? - Remove JEMALLOC_RECYCLE/config_recycle? - MALLOC_STATIC_SIZES is only unset on rare platforms (IA64, Sparc, MIPS, ARM64). Can we use static sizes on all platforms? - Replace assert with MOZ_ASSERT. - Remove JEMALLOC_USES_MAP_ALIGN? - __DECONST is defined twice. - Convert many macros to functions or constants. - Convert tabs to spaces, and a zillion other formatting changes... (maybe later!)

(In reply to Nicholas Nethercote [:njn] from comment #2) > - MALLOC_SYSV is only defined if MALLOC_PRODUCTION is undefined. That seems > really surprising -- surely we want the same semantics always? And can > opt_sysv be removed as well? Yeah this one is really weird. > - Remove MALLOC_BALANCE? Already removed in bug 1364358. > - MALLOC_STATIC_SIZES is only unset on rare platforms (IA64, Sparc, MIPS, > ARM64). Can we use static sizes on all platforms? Sadly no, because those platforms can have different page sizes at runtime. Also, while ia64, sparc and mips are rare, arm64 is going to be an Android target at some point. > - Replace assert with MOZ_ASSERT. FWIW, that will be required once we start using C++ headers (I added <algorithm> in bug 1365194, but in a Windows-only section because including it everywhere created problems wrt assert in some system header (which #define it)) > - Remove JEMALLOC_USES_MAP_ALIGN? Not sure about this one, Oracle is actually making the effort to make Firefox buildable for Solaris again. It was added a long time ago to circumvent an infinite loop in chunk_alloc_mmap (bug 457189). Maybe that loop was fixed otherwise (ISTR we've had something similar fixed semi-recently, maybe that was the same root cause) > - Convert many macros to functions or constants. I want to keep this bug for simple straightforward things. Constants would be fine, functions might be too much, at least in some cases. Probably better in a followup. > - Convert tabs to spaces, and a zillion other formatting changes... (maybe > later!) As mentioned on irc, this can cause problems with blame, although both mercurial and git afaik have flags to ignore whitespace changes in blame (but that's not available on hgweb). While not ideal, maybe we can agree that every new things from now on (like C++ replacements for macros, RAII helpers, etc.) should be formatted with Mozilla style, which would create a little mess in the file. Maybe also functions that are adapted to use those changes if they are changed significantly enough. And once a significant portion of the file is rewritten, then reformat the remainder... none of the options really feel great (status quo, full reformatting, piecemeal reformatting)

(In reply to Mike Hommey [:glandium] from comment #0) > More ideas? |isthreaded| is only ever set to |true|. Fold it out.

(In reply to Julian Seward [:jseward] from comment #4) > (In reply to Mike Hommey [:glandium] from comment #0) > > More ideas? > > |isthreaded| is only ever set to |true|. Fold it out. Removed in bug 1364358 :)

I imagine we can get rid of a bunch of the #defines/typedefs for windows [1] now that we require a more modern compiler. [1] https://dxr.mozilla.org/mozilla-central/rev/41958333867b0f537271dbd4cb4ba9e8a67a85a8/memory/mozjemalloc/jemalloc.c#219-285

It seems like all the #ifdef MOZ_MEMORY stuff, do we ever build mozjemalloc w/o MOZ_MEMORY?

Another one: give names to the 0xe4 and 0xe5 junk/poison constants.

(In reply to Eric Rahm [:erahm] from comment #7) > It seems like all the #ifdef MOZ_MEMORY stuff, do we ever build mozjemalloc > w/o MOZ_MEMORY? Removed in bug 1365191 already. (In reply to Eric Rahm [:erahm] from comment #6) > I imagine we can get rid of a bunch of the #defines/typedefs for windows [1] > now that we require a more modern compiler. > > [1] > https://dxr.mozilla.org/mozilla-central/rev/ > 41958333867b0f537271dbd4cb4ba9e8a67a85a8/memory/mozjemalloc/jemalloc.c#219- > 285 Some of those were removed in bug 1365194.

- Remove support for /etc/malloc.conf.

(In reply to Nicholas Nethercote [:njn] from comment #8) > Another one: give names to the 0xe4 and 0xe5 junk/poison constants. jemalloc's junk constants upstream are now called JEMALLOC_ALLOC_JUNK and JEMALLOC_FREE_JUNK: https://github.com/jemalloc/jemalloc/pull/363

(In reply to Nicholas Nethercote [:njn] from comment #2) > - Convert many macros to functions or constants. Left this out because the patch queue was starting to get deep. Will file a followup.

Comment on attachment 8868847 [details] Bug 1365460 - Remove unused macros from rb.h. https://reviewboard.mozilla.org/r/140434/#review143814

Comment on attachment 8868848 [details] Bug 1365460 - Remove unused qr.h and ql.h headers. https://reviewboard.mozilla.org/r/140436/#review143816

Comment on attachment 8868849 [details] Bug 1365460 - Replace literal 0xe4/0xe5 junk/poison values with constants. https://reviewboard.mozilla.org/r/140438/#review143818

Comment on attachment 8868850 [details] Bug 1365460 - Remove the possibility to disable free() poisoning at runtime. https://reviewboard.mozilla.org/r/140440/#review143822 ::: memory/mozjemalloc/mozjemalloc.cpp:4124 (Diff revision 1) > > if (psize < oldsize) { > /* Fill before shrinking in order avoid a race. */ > - if (opt_poison) { > - memset((void *)((uintptr_t)ptr + size), kAllocPoison, > + memset((void *)((uintptr_t)ptr + size), kAllocPoison, > - oldsize - size); > + oldsize - size); Fix indentation of the second line while here.

Comment on attachment 8868850 [details] Bug 1365460 - Remove the possibility to disable free() poisoning at runtime. https://reviewboard.mozilla.org/r/140440/#review143824

Comment on attachment 8868851 [details] Bug 1365460 - Remove the inline extra malloc options. https://reviewboard.mozilla.org/r/140442/#review143826 ::: memory/mozjemalloc/mozjemalloc.cpp:4745 (Diff revision 1) > pagesize = (size_t) result; > pagesize_mask = (size_t) result - 1; > pagesize_2pow = ffs((int)result) - 1; > #endif > > - for (i = 0; i < 3; i++) { > + for (i = 0; i < 2; i++) { This is a truly bizarre loop.

Comment on attachment 8868852 [details] Bug 1365460 - Remove support for /etc/malloc.conf. https://reviewboard.mozilla.org/r/140444/#review143828 ::: memory/mozjemalloc/mozjemalloc.cpp:4766 (Diff revision 1) > } > MALLOC_OUT: > if (nseen == false) > nreps = 1; > > for (k = 0; k < nreps; k++) { Since you changed |j| to |i|, might as well change |k| to |j|?

Comment on attachment 8868853 [details] Bug 1365460 - Define _malloc_message as a function instead of a pointer to a function. https://reviewboard.mozilla.org/r/140446/#review143830

Comment on attachment 8868854 [details] Bug 1365460 - Replace MOZ_MEMORY_DEBUG, MALLOC_DEBUG and !MALLOC_PRODUCTION with MOZ_DEBUG. https://reviewboard.mozilla.org/r/140448/#review143834 ::: memory/mozjemalloc/mozjemalloc.cpp:140 (Diff revision 1) > -#ifndef MALLOC_PRODUCTION > - /* > * MALLOC_DEBUG enables assertions and other sanity checks, and disables > * inline functions. > */ > # define MALLOC_DEBUG Is MALLOC_DEBUG still useful? Looks like it could also be replaced by MOZ_DEBUG.

Comment on attachment 8868855 [details] Bug 1365460 - Remove runtime support for SysV malloc semantics. https://reviewboard.mozilla.org/r/140450/#review143836 ::: memory/mozjemalloc/mozjemalloc.cpp (Diff revision 1) > } > > num_size = num * size; > if (num_size == 0) { > -#ifdef MALLOC_SYSV > - if ((opt_sysv == false) && ((num == 0) || (size == 0))) This was a weird condition. How could the RHS fail if num_size==0? I guess that's why you removed it. ::: memory/mozjemalloc/mozjemalloc_types.h:58 (Diff revision 1) > typedef struct { > /* > * Run-time configuration settings. > */ > jemalloc_bool opt_abort; /* abort(3) on error? */ > jemalloc_bool opt_junk; /* Fill allocated memory with 0xe4? */ Oh, you missed a s/0xe4/kAllocJunk/ conversion here.

Comment on attachment 8868856 [details] Bug 1365460 - Remove JEMALLOC_RECYCLE/config_recycle, they are always true. https://reviewboard.mozilla.org/r/140452/#review143840

Comment on attachment 8868857 [details] Bug 1365460 - Remove JEMALLOC_MUNMAP/config_munmap, they are always true. https://reviewboard.mozilla.org/r/140454/#review143848

Comment on attachment 8868855 [details] Bug 1365460 - Remove runtime support for SysV malloc semantics. https://reviewboard.mozilla.org/r/140450/#review143836 > This was a weird condition. How could the RHS fail if num_size==0? I guess that's why you removed it. Well, I removed it because that's in a #ifdef we consider always false.

Comment on attachment 8868858 [details] Bug 1365460 - Replace __DECONST with const_cast. https://reviewboard.mozilla.org/r/140456/#review143850

Comment on attachment 8868850 [details] Bug 1365460 - Remove the possibility to disable free() poisoning at runtime. https://reviewboard.mozilla.org/r/140440/#review143822 > Fix indentation of the second line while here. Not totally sure about this one, there's a *lot* of similar indentation, and that would stand out. Maybe we could fix those in one go, even if not touching the tabs?

Comment on attachment 8868859 [details] Bug 1365460 - Expand the __crtInitCritSecAndSpinCount macro and remove it. https://reviewboard.mozilla.org/r/140458/#review143854

Comment on attachment 8868860 [details] Bug 1365460 - Replace assert with MOZ_ASSERT. https://reviewboard.mozilla.org/r/140460/#review143856

Comment on attachment 8868861 [details] Bug 1365460 - Define MOZ_DIAGNOSTIC_ASSERT_ENABLED when MOZ_DIAGNOSTIC_ASSERT does something. https://reviewboard.mozilla.org/r/140462/#review143858 ::: mfbt/Assertions.h:454 (Diff revision 1) > +# ifdef DEBUG > +# define MOZ_DIAGNOSTIC 1 > +# endif > #else > # define MOZ_DIAGNOSTIC_ASSERT MOZ_RELEASE_ASSERT > +# define MOZ_DIAGNOSTIC 1 It would be nice to comment this. (The relevant comment block is about 80 lines up in the file.)

Comment on attachment 8868862 [details] Bug 1365460 - Use MOZ_DIAGNOSTIC_ASSERT where mozjemalloc uses RELEASE_ASSERT. https://reviewboard.mozilla.org/r/140464/#review143862 So these assertions are now Nightly-only? That's a shame. I guess we don't have a way to say "enable this for Nightly and Beta".

Comment on attachment 8868863 [details] Bug 1365460 - Remove JEMALLOC_USES_MAP_ALIGN. https://reviewboard.mozilla.org/r/140466/#review143866

Comment on attachment 8868864 [details] Bug 1365460 - Don't define integer types that MSVC >= 2015 know about. https://reviewboard.mozilla.org/r/140468/#review143870

Comment on attachment 8868865 [details] Bug 1365460 - Remove warning exceptions for MSVC and work around them. https://reviewboard.mozilla.org/r/140470/#review143872

Comment on attachment 8868862 [details] Bug 1365460 - Use MOZ_DIAGNOSTIC_ASSERT where mozjemalloc uses RELEASE_ASSERT. https://reviewboard.mozilla.org/r/140464/#review143862 It would be great to have them on beta, but at some point, as mentioned in the commit message, that will mean it ends up on release too... DevEdition, based on beta, will have MOZ_DIAGNOSTIC set, fwiw.

Comment on attachment 8868866 [details] Bug 1365460 - Avoid unused variables. https://reviewboard.mozilla.org/r/140472/#review143874 ::: memory/mozjemalloc/mozjemalloc.cpp:2868 (Diff revision 2) > chunk->ndirty = 0; > > /* Initialize the map to contain one maximal free untouched run. */ > +#ifdef MALLOC_DECOMMIT > + arena_run_t *run; > run = (arena_run_t *)((uintptr_t)chunk + (arena_chunk_header_npages << You could combine the two lines.

Many nice changes here. And thank you for splitting them up so nicely into separate patches.

Comment on attachment 8868900 [details] Bug 1365460 - Make umax2s support only base 10. https://reviewboard.mozilla.org/r/140524/#review143954

Comment on attachment 8868901 [details] Bug 1365460 - Remove function prototypes but keep necessary forward declarations. https://reviewboard.mozilla.org/r/140526/#review143956 Whoa.

Comment on attachment 8868861 [details] Bug 1365460 - Define MOZ_DIAGNOSTIC_ASSERT_ENABLED when MOZ_DIAGNOSTIC_ASSERT does something. https://reviewboard.mozilla.org/r/140462/#review144180 r=me with the below accepted as-is or with a better counter-proposal. ::: mfbt/Assertions.h:368 (Diff revision 3) > + * MOZ_DIAGNOSTIC is defined when MOZ_DIAGNOSTIC_ASSERT is like > + * MOZ_RELEASE_ASSERT rather than MOZ_ASSERT. This seems like a reasonable idea; I'd be a little happier if this was called something like `MOZ_DIAGNOSTIC_ASSERTIONS_ACTIVE`. `MOZ_DIAGNOSTIC` is...non-descriptive. WDYT?

Per irc: MOZ_DIAGNOSTIC_ASSERT_ENABLED.

Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/1a4fb12801bd Remove unused macros from rb.h. r=njn https://hg.mozilla.org/integration/autoland/rev/cb9002ac2442 Remove unused qr.h and ql.h headers. r=njn https://hg.mozilla.org/integration/autoland/rev/116c3ddd7599 Replace literal 0xe4/0xe5 junk/poison values with constants. r=njn https://hg.mozilla.org/integration/autoland/rev/c6a0c9b24771 Remove the possibility to disable free() poisoning at runtime. r=njn https://hg.mozilla.org/integration/autoland/rev/0ee6b9a3d8b8 Remove the inline extra malloc options. r=njn https://hg.mozilla.org/integration/autoland/rev/a57b93dbf8c8 Remove support for /etc/malloc.conf. r=njn https://hg.mozilla.org/integration/autoland/rev/f520dbc8c753 Define _malloc_message as a function instead of a pointer to a function. r=njn https://hg.mozilla.org/integration/autoland/rev/3d3d5c1b5e4f Replace MOZ_MEMORY_DEBUG, MALLOC_DEBUG and !MALLOC_PRODUCTION with MOZ_DEBUG. r=njn https://hg.mozilla.org/integration/autoland/rev/aedb6691ec3c Remove runtime support for SysV malloc semantics. r=njn https://hg.mozilla.org/integration/autoland/rev/69e5a5ad8292 Remove JEMALLOC_RECYCLE/config_recycle, they are always true. r=njn https://hg.mozilla.org/integration/autoland/rev/1c5bd0d9ea82 Remove JEMALLOC_MUNMAP/config_munmap, they are always true. r=njn https://hg.mozilla.org/integration/autoland/rev/ddeff37db66e Replace __DECONST with const_cast. r=njn https://hg.mozilla.org/integration/autoland/rev/5e3fe541856a Expand the __crtInitCritSecAndSpinCount macro and remove it. r=njn https://hg.mozilla.org/integration/autoland/rev/3f4357f9aa1e Replace assert with MOZ_ASSERT. r=njn https://hg.mozilla.org/integration/autoland/rev/c174e6090f9e Define MOZ_DIAGNOSTIC_ASSERT_ENABLED when MOZ_DIAGNOSTIC_ASSERT does something. r=froydnj https://hg.mozilla.org/integration/autoland/rev/1af2e006c4c1 Use MOZ_DIAGNOSTIC_ASSERT where mozjemalloc uses RELEASE_ASSERT. r=njn https://hg.mozilla.org/integration/autoland/rev/a26c500b98ab Remove JEMALLOC_USES_MAP_ALIGN. r=njn https://hg.mozilla.org/integration/autoland/rev/0589ac03b763 Don't define integer types that MSVC >= 2015 know about. r=njn https://hg.mozilla.org/integration/autoland/rev/4b56d482d8e8 Remove warning exceptions for MSVC and work around them. r=njn https://hg.mozilla.org/integration/autoland/rev/7ed98992f409 Avoid unused variables. r=njn https://hg.mozilla.org/integration/autoland/rev/d8e658d79508 Make umax2s support only base 10. r=njn https://hg.mozilla.org/integration/autoland/rev/0a7b45d745ce Remove function prototypes but keep necessary forward declarations. r=njn

(In reply to Mike Hommey [:glandium] from comment #104) > Per irc: MOZ_DIAGNOSTIC_ASSERT_ENABLED. Excellent! I had had exactly that thought overnight.

https://hg.mozilla.org/mozilla-central/rev/1a4fb12801bd https://hg.mozilla.org/mozilla-central/rev/cb9002ac2442 https://hg.mozilla.org/mozilla-central/rev/116c3ddd7599 https://hg.mozilla.org/mozilla-central/rev/c6a0c9b24771 https://hg.mozilla.org/mozilla-central/rev/0ee6b9a3d8b8 https://hg.mozilla.org/mozilla-central/rev/a57b93dbf8c8 https://hg.mozilla.org/mozilla-central/rev/f520dbc8c753 https://hg.mozilla.org/mozilla-central/rev/3d3d5c1b5e4f https://hg.mozilla.org/mozilla-central/rev/aedb6691ec3c https://hg.mozilla.org/mozilla-central/rev/69e5a5ad8292 https://hg.mozilla.org/mozilla-central/rev/1c5bd0d9ea82 https://hg.mozilla.org/mozilla-central/rev/ddeff37db66e https://hg.mozilla.org/mozilla-central/rev/5e3fe541856a https://hg.mozilla.org/mozilla-central/rev/3f4357f9aa1e https://hg.mozilla.org/mozilla-central/rev/c174e6090f9e https://hg.mozilla.org/mozilla-central/rev/1af2e006c4c1 https://hg.mozilla.org/mozilla-central/rev/a26c500b98ab https://hg.mozilla.org/mozilla-central/rev/0589ac03b763 https://hg.mozilla.org/mozilla-central/rev/4b56d482d8e8 https://hg.mozilla.org/mozilla-central/rev/7ed98992f409 https://hg.mozilla.org/mozilla-central/rev/d8e658d79508 https://hg.mozilla.org/mozilla-central/rev/0a7b45d745ce