Last Comment Bug 738176 - jemalloc is not entirely disabled on OSX 10.5
: jemalloc is not entirely disabled on OSX 10.5
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: jemalloc3 702250
  Show dependency treegraph
 
Reported: 2012-03-22 02:52 PDT by Mike Hommey [:glandium]
Modified: 2012-04-05 11:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs. (11.71 KB, patch)
2012-03-22 10:43 PDT, Mike Hommey [:glandium]
justin.lebar+bug: review+
Details | Diff | Review
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs. (18.86 KB, patch)
2012-03-28 05:55 PDT, Mike Hommey [:glandium]
justin.lebar+bug: review+
khuey: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2012-03-22 02:52:19 PDT
All allocations going through mozalloc are using jemalloc. And as the jemalloc zone is not registered, this means the system zone allocator can't find these allocations if the corresponding pointers end up being sent to it (unlikely, but still dangerous).

Also, je_malloc_usable_size_in_advance returns sizes for jemalloc, which may induce sqlite to over-allocate with the system allocator.
Comment 1 Mike Hommey [:glandium] 2012-03-22 10:43:36 PDT
Created attachment 608389 [details] [diff] [review]
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs.

This makes mozalloc go through the system zone allocator, which will properly go back in jemalloc if it is registered, or dtrt if not. To achieve that, we need to remove support in mozalloc for APIs the zone allocator doesn't support. Which is okay since we don't use them anyways.

Successful on try (if you forget the blueness ; the first try forgot the .def for mozglue on windows, so it failed to build, and the second try just fixed that):
https://tbpl.mozilla.org/?tree=Try&rev=2b7c7a33b17c
https://tbpl.mozilla.org/?tree=Try&rev=446d1093bdb6
Comment 2 Mike Hommey [:glandium] 2012-03-28 05:55:18 PDT
Created attachment 610088 [details] [diff] [review]
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs.

This does even more cleanup.

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=0c23d85d37b9
Android talos red is infrastructure (cleanup failed)
Windows opt red is infrastructure (upload symbols failed)
Moth orange is on builds not even using jemalloc.
Comment 3 Justin Lebar (not reading bugmail) 2012-03-31 16:07:05 PDT
Do you think it would make sense to put assert(!macos_10_5) in jemalloc's public functions?  I don't think tinderbox does debug jemalloc builds, but at least someone would catch this failure.
Comment 4 Justin Lebar (not reading bugmail) 2012-03-31 16:16:11 PDT
> +#if defined(MOZ_MEMORY_LINUX)
> +static inline void jemalloc_purge_freed_pages() { }
> +#else
>  void    jemalloc_purge_freed_pages();
> +#endif

Hm.  This precludes defining MALLOC_DOUBLE_PURGE on Linux.  Which is fine -- it's only needed on Mac.  But could you indicate in the MALLOC_DOUBLE_PURGE comment that this footgun exists?  Otherwise, you could just make this symbol weak on Linux, which shouldn't hurt anything since we're not going to call it.
Comment 5 Justin Lebar (not reading bugmail) 2012-03-31 16:21:04 PDT
Comment on attachment 610088 [details] [diff] [review]
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs.

r=me for jemalloc itself, but I'm not at all comfortable reviewing the build changes.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-03 08:38:38 PDT
Comment on attachment 610088 [details] [diff] [review]
Completely disable jemalloc when it's supposed to be disabled on OSX, and cleanup exposed APIs.

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

Why do we no longer need the weak symbol stuff?
Comment 7 Mike Hommey [:glandium] 2012-04-03 08:50:49 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Why do we no longer need the weak symbol stuff?

Because it's directly in jemalloc.h.
Comment 9 :Ehsan Akhgari (out sick) 2012-04-05 11:29:45 PDT
http://hg.mozilla.org/mozilla-central/rev/26eb0d5a95a6

Note You need to log in before you can comment on or make changes to this bug.