Enable jemalloc debug on debug builds

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed)

Details

Attachments

(4 attachments)

No description provided.
Note this can't land because it unveils bugs.
Attachment #8576474 - Flags: review?(n.nethercote)
Comment on attachment 8576474 [details] [diff] [review]
Enable jemalloc debug on debug builds

Review of attachment 8576474 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine but I'm confused by the "unveils bugs" comment. Does that mean this can't land until those bugs are resolved? Should they be filed and marked as blocking this bug?
Attachment #8576474 - Flags: review?(n.nethercote) → review+
Depends on: 1142412
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Comment on attachment 8576474 [details] [diff] [review]
> Enable jemalloc debug on debug builds
> 
> Review of attachment 8576474 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems fine but I'm confused by the "unveils bugs" comment. Does that mean
> this can't land until those bugs are resolved?

Yes.

> Should they be filed and marked as blocking this bug?

Done for the main contender. I'll file the other Windows one I saw, and push a try run on all platforms to see if there are others on non-Windows.
I'm dumb. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5801ac4428be

(In reply to Mike Hommey [:glandium] from comment #3)
> Done for the main contender. I'll file the other Windows one I saw, and push
> a try run on all platforms to see if there are others on non-Windows.

It turns out the others might have been the same one, but without a proper backtrace, because a try with the main contender worked around is all green.
When built with --enable-debug, jemalloc3 makes headers define functions that
are normally inlined, and that prevents unified sources from working.
Attachment #8646775 - Flags: review?(mshal)
Comment on attachment 8646775 [details] [diff] [review]
Don't build jemalloc3 with unified sources when built with --enable-debug

>When built with --enable-debug, jemalloc3 makes headers define functions that
>are normally inlined, and that prevents unified sources from working.

>diff --git a/memory/jemalloc/moz.build b/memory/jemalloc/moz.build
>+if CONFIG['MOZ_DEBUG']:
>+    maybe_unified_sources = SOURCES
>+else:
>+    maybe_unified_sources = UNIFIED_SOURCES

Can you add a comment here? The "When built with --enable-debug" message should suffice.
Attachment #8646775 - Flags: review?(mshal) → review+
Blocks: 1201802
Part 1: Enable jemalloc debug on debug builds.

Mike, I rebased your jemalloc debug patch that was backed out last year (comment 9). Also, jemalloc4 has resolved the arena_purge symbol name conflict in ctl.c so ctl.c can now be built in unified mode.

AFAICT nothing blows up with debug mozjemalloc or jemalloc4. Here are some Try runs with a couple test failures and timeouts, but I don't think they are related to jemalloc debug problems. Similar timeouts appear in a Try run of the vanilla mozilla-central base revision below.

mozjemalloc debug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb535bee8af664b879742b303ee41f43f4e43f7d&selectedJob=32810696

--enable-jemalloc=4 debug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f49c3eeb11e3ca49997bc2fc066b8fdcad92171&selectedJob=32809043

mozilla-central base revision:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d196017eb55beadc7344ef813918a693644fcdab
Attachment #8819626 - Flags: review?(mh+mozilla)
Part 2: Enable jemalloc opt_abort in release builds of Nightly and Aurora?

When opt_abort is true, jemalloc will abort in some error cases such as munmap/VirtualFree failures, pthread and mutex failures, and arenas_extend OOM. These error cases might be unlikely enough or serious enough to keep these aborts enabled on the Beta or Release channels, but I limited this change to Nightly and Aurora (with #ifdef NIGHTLY_BUILD) to play it safe because I realize this proposed change might be controversial. :)

mozjemalloc's opt_abort usage:

http://searchfox.org/mozilla-central/search?q=symbol:_ZL9opt_abort

jemalloc4's opt_abort usage:

http://searchfox.org/mozilla-central/search?q=symbol:M_fb9f3db96e72f728fa5a7a8d7eef68e55d4590bc
Attachment #8819628 - Flags: review?(mh+mozilla)
Comment on attachment 8819626 [details] [diff] [review]
part-1-jemalloc-enable-debug.patch

Review of attachment 8819626 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/jemalloc/moz.build
@@ -36,5 @@
>  
> -SOURCES += [
> -    # This file cannot be built in unified mode because of symbol clash on arena_purge.
> -    'src/src/ctl.c',
> -]

Can you move this to a separate patch?
Attachment #8819626 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 8819626 [details] [diff] [review]
> part-1-jemalloc-enable-debug.patch
> 
> Review of attachment 8819626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/jemalloc/moz.build
> @@ -36,5 @@
> >  
> > -SOURCES += [
> > -    # This file cannot be built in unified mode because of symbol clash on arena_purge.
> > -    'src/src/ctl.c',
> > -]
> 
> Can you move this to a separate patch?

In fact, you could just cherry-pick the old landing, and land this part on top, + a CLOBBER update.
Comment on attachment 8819628 [details] [diff] [review]
part-2-jemalloc-enable-opt_abort.patch

Review of attachment 8819628 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/build/jemalloc_config.cpp
@@ +31,5 @@
>  #else
>  #define MOZ_MALLOC_BUILD_OPTIONS ",junk:free"
>  #endif
>  
> +#ifdef NIGHTLY_BUILD

Mmmmm maybe ifdef DEBUG to start small, and file a followup to see what to do on other builds?

@@ +32,5 @@
>  #define MOZ_MALLOC_BUILD_OPTIONS ",junk:free"
>  #endif
>  
> +#ifdef NIGHTLY_BUILD
> +#define MOZ_MALLOC_OPTIONS "abort:true,narenas:1,tcache:false"

Please make it so that changing the malloc options doesn't require editing two lines.
Attachment #8819628 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #13)
> In fact, you could just cherry-pick the old landing, and land this part on
> top, + a CLOBBER update.

I'm not sure I understand. Are you suggesting I revert the backout changesets (from comment 9)?


> > +#ifdef NIGHTLY_BUILD
> 
> Mmmmm maybe ifdef DEBUG to start small, and file a followup to see what to
> do on other builds?

opt_abort is already true in debug builds. 

In mozjemalloc when MALLOC_PRODUCTION is not defined (i.e. when MOZ_MEMORY_DEBUG is not defined i.e. when MOZ_DEBUG is not defined):

http://searchfox.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#1314

In jemalloc4 when when JEMALLOC_DEBUG is #defined:

http://searchfox.org/mozilla-central/source/memory/jemalloc/src/src/jemalloc.c#13


> > +#ifdef NIGHTLY_BUILD
> > +#define MOZ_MALLOC_OPTIONS "abort:true,narenas:1,tcache:false"
> 
> Please make it so that changing the malloc options doesn't require editing
> two lines.

OK.
(In reply to Chris Peterson [:cpeterson] from comment #15)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > In fact, you could just cherry-pick the old landing, and land this part on
> > top, + a CLOBBER update.
> 
> I'm not sure I understand. Are you suggesting I revert the backout
> changesets (from comment 9)?

I'm suggesting you re-apply/cherry-pick (git terminology)/graft (mercurial terminology) the changesets from comment 8.
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ef2f6402ac
Part 1: Don't build jemalloc4 with unified sources when built with --enable-debug. r=mshal
https://hg.mozilla.org/integration/mozilla-inbound/rev/e35cf42e5c92
Part 2: Enable jemalloc debug on debug builds. r=njn
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b315b98f03a
Part 3: jemalloc4 no longer requires ctl.c to be built in non-unified mode in debug builds. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/66465b966647
Part 4: Clobber to force jemalloc to rerun configure. r=glandium
Chris, I'm assuming you aim to keep this in 53, but if you are thinking of uplift please let us know.
Mike, do you think we should enable debug jemalloc to Aurora 52?
Flags: needinfo?(cpeterson) → needinfo?(mh+mozilla)
Considering it's not the default, no.
Flags: needinfo?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.