1107381 - Remove some #ifdef goop in favor of jemalloc3-style statically known branches.

Status: RESOLVED WONTFIX

(Core :: Memory Allocator, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

mozjemalloc enables/disables a lot of code based on local #ifdefs. jemalloc3 does away with this in favor of statically known branches. For instance, instead of

#ifdef JEMALLOC_STATS
  /* Adjust stats. */
#endif

they have

if (config_stats) {
  /* Adjust stats. */
}

This has a few advantages:
(*) All code is compiled unconditionally, so it's harder to break compilation
    for non-default configurations.
(*) Unlike #ifdefs, these branches are indented the same as surrounding code,
    making it more obvious we're in a conditional block.
(*) They can be easily integrated with runtime decisions.

It also has a few disadvantages:
(*) Since all code is compiled unconditionally, commonly used structures
    may grow additional fields for rarely used configurations.
(*) The indentation level may increase. Since jemalloc uses 8-space tabs and
    an 80-character line limit, this can be pretty limiting.
(*) Some things are done more easily using #ifdefs than directly in C.

I'm not proposing that we remove all our #ifdef goop - that would probably be a waste of time. However, I think it would be easier to port our decommit logic to jemalloc3 if it used this style. MALLOC_STATS, MALLOC_DECOMMIT and MALLOC_DOUBLE_PURGE are all pretty entwined, so I converted them over to config_stats (matching jemalloc3), config_decommit and config_double_purge respectively.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Remove some mozjemalloc ifdef goop in favor of jemalloc3-style statically known branches.

This is a big patch, but most of it is context and indentation changes (and resulting line rewrapping).

The only change in behavior should be in arena_purge(), where we were incrementing the counters for both decommit and madvise despite statically knowing which one we do. This logic also didn't compile on Windows (but was working because Windows uses MALLOC_DECOMMIT). I changed it to use pages_purge(), which should match what it had before (but fixed to avoid resetting/decommitting across chunk boundaries on Windows).

This also exposed some warnings in jemalloc_purge_freed_pages_impl() caused by not declaring |extent_node_t *node| at the top of its block. These warnings were presumably present on OSX previously, we just didn't notice.

Having config_decommit and config_double_purge does mean that a few structures grow some extra fields (namely arena_stats_s, arena_chunk_s and arena_bin_s). I could imagine this causing an increase in memory usage for accounting, though it will at least make platforms consistent (32-bit versus 64-bit differences aside). We probably had the fields added by config_stats on all platforms already.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7c726ca5b4a6

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8531845 [details] [diff] [review]
Remove some mozjemalloc ifdef goop in favor of jemalloc3-style statically known branches.

We're hoping to switch to jemalloc 3 very soon now, so let's not change mozjemalloc at this point.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Okay. I think this was worth doing for porting the decommit logic over - but you can just apply the patch locally to work on that.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Didn't mean to change that :\