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)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: jemalloc3
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)
Attachment #608389 - Flags: review?(justin.lebar+bug) → review+
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)
Attachment #608389 - Attachment is obsolete: true
Attachment #608389 - Flags: review?(khuey)
Attachment #610088 - Flags: review?(khuey)
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.
> +#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 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?
(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.
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.

Attachment

General

Created:
Updated:
Size: