Closed
Bug 738176
Opened 13 years ago
Closed 13 years ago
jemalloc is not entirely disabled on OSX 10.5
Categories
(Core :: Memory Allocator, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
justin.lebar+bug
:
review+
khuey
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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
Attachment #608389 -
Flags: review?(khuey)
Attachment #608389 -
Flags: review?(justin.lebar+bug)
Updated•13 years ago
|
Attachment #608389 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 2•13 years ago
|
||
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.
Attachment #610088 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•13 years ago
|
Attachment #608389 -
Attachment is obsolete: true
Attachment #608389 -
Flags: review?(khuey)
Updated•13 years ago
|
Attachment #610088 -
Flags: review?(khuey)
Comment 3•13 years ago
|
||
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•13 years ago
|
||
> +#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•13 years ago
|
||
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.
Attachment #610088 -
Flags: review?(justin.lebar+bug) → review+
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?
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Attachment #610088 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 8•13 years ago
|
||
Target Milestone: --- → mozilla14
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•