Closed Bug 1363992 Opened 7 years ago Closed 7 years ago

Remove jemalloc 4

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

(3 files)

Bug 1363992 - Remove support for system jemalloc.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1363992 - Remove support for making jemalloc4 the default.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
Bug 1363992 - Remove jemalloc 4.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
We've tried to get off mozjemalloc for, apparently, close to 5 years (date of the filing of bug 762449). We've had memory usage regressions (like bug 1219914), and we've had perf regressions as per talos numbers (things like bug 1138999), and those have never gone away.

My best bet at this point is that it's "just" a consequence of the difference in memory layout due to the differences in how the allocator works. That doesn't make it more okay.

Many updates to recent versions of jemalloc have been painful (usually breaking everything except linux), and the current trunk of the jemalloc dev branch is not making things any better (see bug 1357301).

Furthermore, bug 1361258 is starting to modify mozjemalloc with new features for stylo, further deepening the differences between both allocators. While what is added in bug 1361258 is possible to implement for jemalloc 4, the burden of continuing to maintain both allocators is not really appealing considering the perspective of ever switching.

As much as it pains me, it's time to admit that switching to jemalloc >= 3 is not going to happen, and pull the plug once and for all.
Jan, you should know about the first patch here, as it impacts you.
Comment on attachment 8866675 [details]
Bug 1363992 - Remove support for system jemalloc.

Looks/builds/runs fine. As expected about:memory (after pressing Measure button) once again shows the following:

  WARNING: the 'heap-allocated' memory reporter does not work for this platform and/or configuration. This means that 'heap-unclassified' is not shown and the 'explicit' tree shows less memory than it should. 

while MALLOC_CONF=stats_print:true (on 12.0-CURRENT) shows

  ___ Begin jemalloc statistics ___
  Version: 4.5.0-0-g04380e79f1e2428bd0ad000bbc6e3d2dfc6b66a5
  Assertions disabled
  config.malloc_conf: ""
  Run-time option settings:
    opt.abort: false
    opt.lg_chunk: 21
    opt.dss: "secondary"
 -  opt.narenas: 1
 +  opt.narenas: 32
    opt.purge: "ratio"
    opt.lg_dirty_mult: 3 (arenas.lg_dirty_mult: 3)
 -  opt.junk: "free"
 +  opt.junk: "false"
    opt.quarantine: 0
    opt.redzone: false
    opt.zero: false
    opt.utrace: false
    opt.xmalloc: false
 -  opt.tcache: false
 +  opt.tcache: true
    opt.lg_tcache_max: 15
    opt.stats_print: true
 -Arenas: 1
 +Arenas: 32
  Min active:dirty page ratio per arena: 8:1
  Quantum size: 16
  Page size: 4096
  Maximum thread-cached size class: 32768
 -Allocated: 58066080, active: 65863680, metadata: 3053720, resident: 77119488, mapped: 98566144, retained: 0
 -Current active ceiling: 67108864
 +Allocated: 61604344, active: 73375744, metadata: 6238400, resident: 109862912, mapped: 184549376, retained: 0
 +Current active ceiling: 132120576
  [...]

(- lines are from yesterday build. Both -/+ builds are configured close to what downstream uses except for bug 1356991 dogfooding.)
Attachment #8866675 - Flags: feedback+
Comment on attachment 8866677 [details]
Bug 1363992 - Remove jemalloc 4.

MOZ_SUBCONFIGURE_JEMALLOC() left in old-configure.in breaks build. And some stuff still references jemalloc4

  $ git grep -lF memory/jemalloc
  .clang-format-ignore
  js/src/make-source-package.sh
  memory/build/moz.build
  toolkit/content/license.html
  tools/rewriting/ThirdPartyPaths.txt
(In reply to Jan Beich from comment #5)
> Comment on attachment 8866675 [details]
> Bug 1363992 - Remove support for system jemalloc.
> 
> Looks/builds/runs fine. As expected about:memory (after pressing Measure
> button) once again shows the following:
> 
>   WARNING: the 'heap-allocated' memory reporter does not work for this
> platform and/or configuration. This means that 'heap-unclassified' is not
> shown and the 'explicit' tree shows less memory than it should.

Note that at some point you'll be better off enabling mozjemalloc. AFAICK, it currently doesn't build on freebsd, but afaict, all the os support is actually in the code, just ifdefed on the wrong thing (essentially, !MOZ_MEMORY means freebsd, but maybe you'd be good with the posix pthread stuff too).
(In reply to Mike Hommey [:glandium] from comment #10)
>  Note that at some point you'll be better off enabling mozjemalloc.

mozjemalloc builds fine but crashes, see bug 1153683. !MOZ_MEMORY path via https://pastebin.mozilla.org/9021457 hangs instead.
Tor Browser seems to use opt.redzone which is N/A in mozjemalloc. Otherwise, malloc_conf="abort:true,junk:true" can be converted to _malloc_options="AJ".

https://trac.torproject.org/projects/tor/ticket/10281
Redzones are not really useful if you run ASAN. (also note that upstream jemalloc trunk removed support for them)
Comment on attachment 8866675 [details]
Bug 1363992 - Remove support for system jemalloc.

https://reviewboard.mozilla.org/r/138280/#review142790

::: memory/build/mozmemory_wrap.h:157
(Diff revision 2)
> +#  ifdef XP_WIN
> +#    define mozmem_dup_impl(a)      wrap_ ## a
> +#  endif
> +#endif
>  
>  /* All other jemalloc3 functions are prefixed with "je_", except when

Is this comment about jemalloc3 accurate?
Attachment #8866675 - Flags: review?(n.nethercote) → review+
Comment on attachment 8866676 [details]
Bug 1363992 - Remove support for making jemalloc4 the default.

https://reviewboard.mozilla.org/r/138282/#review142796

::: memory/build/moz.build
(Diff revision 2)
>  
>  # Keep jemalloc separated when mozglue is statically linked
>  if CONFIG['MOZ_MEMORY'] and CONFIG['OS_TARGET'] in ('WINNT', 'Darwin', 'Android'):
>      FINAL_LIBRARY = 'mozglue'
> -
> -if CONFIG['MOZ_REPLACE_MALLOC'] and CONFIG['OS_TARGET'] == 'Darwin':

This removal is surprising because the condition doesn't involve jemalloc4...

::: memory/replace/moz.build:12
(Diff revision 2)
>  DIRS += [
>      'logalloc',
>      'replace',
>  ]
>  
> -# Build jemalloc3 as a replace-malloc lib when building with mozjemalloc
> +# Build jemalloc4 as a replace-malloc lib when building with mozjemalloc

Is this comment correct?
Attachment #8866676 - Flags: review?(n.nethercote) → review+
Comment on attachment 8866677 [details]
Bug 1363992 - Remove jemalloc 4.

https://reviewboard.mozilla.org/r/138284/#review142798
Attachment #8866677 - Flags: review?(n.nethercote) → review+
The patches look ok.

Can you please send an advance notice to dev-platform about this? Knowledge about this should be spread widely. E.g. I think some people might have been assuming jemalloc4 would be enabled at some point, e.g. for JS heap separation.
BTW, I give you a gold star for all your efforts to get it working! :)
(In reply to Nicholas Nethercote [:njn] from comment #14)
> Comment on attachment 8866675 [details]
> Bug 1363992 - Remove support for system jemalloc.
> 
> https://reviewboard.mozilla.org/r/138280/#review142790
> 
> ::: memory/build/mozmemory_wrap.h:157
> (Diff revision 2)
> > +#  ifdef XP_WIN
> > +#    define mozmem_dup_impl(a)      wrap_ ## a
> > +#  endif
> > +#endif
> >  
> >  /* All other jemalloc3 functions are prefixed with "je_", except when
> 
> Is this comment about jemalloc3 accurate?

It's outdated, and even better, that je_() macro was only used in files removed in the 3rd patch, so both the comment and the macro can be removed.

(In reply to Nicholas Nethercote [:njn] from comment #15)
> Comment on attachment 8866676 [details]
> Bug 1363992 - Remove support for making jemalloc4 the default.
> 
> https://reviewboard.mozilla.org/r/138282/#review142796
> 
> ::: memory/build/moz.build
> (Diff revision 2)
> >  
> >  # Keep jemalloc separated when mozglue is statically linked
> >  if CONFIG['MOZ_MEMORY'] and CONFIG['OS_TARGET'] in ('WINNT', 'Darwin', 'Android'):
> >      FINAL_LIBRARY = 'mozglue'
> > -
> > -if CONFIG['MOZ_REPLACE_MALLOC'] and CONFIG['OS_TARGET'] == 'Darwin':
> 
> This removal is surprising because the condition doesn't involve jemalloc4...

Yeah, there should have been a condition involving jemalloc4 in the first place, but the corresponding code (in zone.c) does have one, and is being removed in this patch.

> ::: memory/replace/moz.build:12
> (Diff revision 2)
> >  DIRS += [
> >      'logalloc',
> >      'replace',
> >  ]
> >  
> > -# Build jemalloc3 as a replace-malloc lib when building with mozjemalloc
> > +# Build jemalloc4 as a replace-malloc lib when building with mozjemalloc
> 
> Is this comment correct?

It is, at the point of this patch, but removed in the next one.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/cf32c1cbb504
Remove support for system jemalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/0e6e8a7b9973
Remove support for making jemalloc4 the default. r=njn
https://hg.mozilla.org/integration/autoland/rev/faae5fbdd01d
Remove jemalloc 4. r=njn
(In reply to Nicholas Nethercote [:njn] from comment #17)
> E.g. I think some people might have been assuming jemalloc4 would
> be enabled at some point, e.g. for JS heap separation.

Indeed! Time for plan B (experiment with PartitionAlloc)
I see from the dev-platform mail that Mike thinks bug 1361258 can be a foundation for doing this inside mozjemalloc. That works too if it'll get us there faster.
Depends on: 1367695
Blocks: 1369658
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: