Closed Bug 1365460 Opened 7 years ago Closed 7 years ago

More mozjemalloc code cleanup

Categories

(Core :: Memory Allocator, enhancement)

Product:
Core
Component:
Memory Allocator
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(22 files)

Bug 1365460 - Remove unused macros from rb.h.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove unused qr.h and ql.h headers.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Replace literal 0xe4/0xe5 junk/poison values with constants.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove the possibility to disable free() poisoning at runtime.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove the inline extra malloc options.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove support for /etc/malloc.conf.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Define _malloc_message as a function instead of a pointer to a function.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Replace MOZ_MEMORY_DEBUG, MALLOC_DEBUG and !MALLOC_PRODUCTION with MOZ_DEBUG.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove runtime support for SysV malloc semantics.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove JEMALLOC_RECYCLE/config_recycle, they are always true.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove JEMALLOC_MUNMAP/config_munmap, they are always true.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Replace __DECONST with const_cast.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Expand the __crtInitCritSecAndSpinCount macro and remove it.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Replace assert with MOZ_ASSERT.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Define MOZ_DIAGNOSTIC_ASSERT_ENABLED when MOZ_DIAGNOSTIC_ASSERT does something.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1365460 - Use MOZ_DIAGNOSTIC_ASSERT where mozjemalloc uses RELEASE_ASSERT.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove JEMALLOC_USES_MAP_ALIGN.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Don't define integer types that MSVC >= 2015 know about.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove warning exceptions for MSVC and work around them.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Avoid unused variables.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Make umax2s support only base 10.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1365460 - Remove function prototypes but keep necessary forward declarations.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
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?
Flags: needinfo?(n.nethercote)
Flags: needinfo?(jseward)
Flags: needinfo?(erahm)
- 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!)
Flags: needinfo?(n.nethercote)
(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.
Flags: needinfo?(jseward)
(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
Flags: needinfo?(erahm)
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.
Assignee: nobody → mh+mozilla
Comment on attachment 8868847 [details]
Bug 1365460 - Remove unused macros from rb.h.

https://reviewboard.mozilla.org/r/140434/#review143814
Attachment #8868847 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868848 [details]
Bug 1365460 - Remove unused qr.h and ql.h headers.

https://reviewboard.mozilla.org/r/140436/#review143816
Attachment #8868848 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868849 [details]
Bug 1365460 - Replace literal 0xe4/0xe5 junk/poison values with constants.

https://reviewboard.mozilla.org/r/140438/#review143818
Attachment #8868849 - Flags: review?(n.nethercote) → review+
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
Attachment #8868850 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868851 - Flags: review?(n.nethercote) → review+
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|?
Attachment #8868852 - Flags: review?(n.nethercote) → review+
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
Attachment #8868853 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868854 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868855 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868856 [details]
Bug 1365460 - Remove JEMALLOC_RECYCLE/config_recycle, they are always true.

https://reviewboard.mozilla.org/r/140452/#review143840
Attachment #8868856 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868857 [details]
Bug 1365460 - Remove JEMALLOC_MUNMAP/config_munmap, they are always true.

https://reviewboard.mozilla.org/r/140454/#review143848
Attachment #8868857 - Flags: review?(n.nethercote) → review+
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
Attachment #8868858 - Flags: review?(n.nethercote) → review+
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
Attachment #8868859 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868860 [details]
Bug 1365460 - Replace assert with MOZ_ASSERT.

https://reviewboard.mozilla.org/r/140460/#review143856
Attachment #8868860 - Flags: review?(n.nethercote) → review+
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".
Attachment #8868862 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868863 [details]
Bug 1365460 - Remove JEMALLOC_USES_MAP_ALIGN.

https://reviewboard.mozilla.org/r/140466/#review143866
Attachment #8868863 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868864 [details]
Bug 1365460 - Don't define integer types that MSVC >= 2015 know about.

https://reviewboard.mozilla.org/r/140468/#review143870
Attachment #8868864 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868865 [details]
Bug 1365460 - Remove warning exceptions for MSVC and work around them.

https://reviewboard.mozilla.org/r/140470/#review143872
Attachment #8868865 - Flags: review?(n.nethercote) → review+
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.
Attachment #8868866 - Flags: review?(n.nethercote) → review+
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
Attachment #8868900 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868901 [details]
Bug 1365460 - Remove function prototypes but keep necessary forward declarations.

https://reviewboard.mozilla.org/r/140526/#review143956

Whoa.
Attachment #8868901 - Flags: review?(n.nethercote) → review+
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?
Attachment #8868861 - Flags: review?(nfroyd) → review+
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.
Blocks: 1369626
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: