Remove custom chunk allocation

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
+++ This bug was initially created as a clone of Bug #671702 +++

Currently xpconnect defines a custom GC chunk allocator so posix_memalign provided by jemalloc can be used for chunk allocation. However, with 1MB GC chunks jemalloc's posix_memalign effectively wraps mmap/VirtualAlloc. So using posix_memalign just imposes an extra overhead on chunk allocation in the form of jemalloc lock taken during the allocation and an allocated red-black tree node that jemalloc use to track the chunk.

To avoid this overhead and for code simplicity we should remove this custom allocator and the corresponding internal API from the JS engine. If at some point, perhaps with smaller GC chunks, we may need to reconsider using jemalloc, we can add the corresponding support directly to JS engine.
(Assignee)

Comment 1

6 years ago
Created attachment 558133 [details] [diff] [review]
v1

The patch removes js::GCChunkAllocator and its users and turns jsapi-test/testGCChunkAlloc into OOM checker.
Assignee: general → igor
Attachment #558133 - Flags: review?(nnethercote)
Comment on attachment 558133 [details] [diff] [review]
v1

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

> To avoid this overhead and for code simplicity we should remove this custom
> allocator and the corresponding internal API from the JS engine.

The simplicity improvements are good, I suspect the overhead reduction is negligible.

The main concern I have is this: what happens when you free a 1MB chunk directly with munmap/VirtualFree?   Is the behaviour the same as when you free a 1MB chunk with free()?  pbiggar, jlebar, do you know?  r=nnethercote once we have an acceptable answer to this question.

BTW, with the patch applied I couldn't build the browser because testGCChunkAlloc.cpp was renamed, but maybe that's a shortcoming of |hg diff|.
Attachment #558133 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 3

6 years ago
(In reply to Nicholas Nethercote [:njn] from comment #2)
> The main concern I have is this: what happens when you free a 1MB chunk
> directly with munmap/VirtualFree?   Is the behaviour the same as when you
> free a 1MB chunk with free()?

For 1MB chunks jemalloc free, http://mxr.mozilla.org/mozilla-central/source/memory/jemalloc/jemalloc.c#6244, calls huge_dalloc. The latter first remove the red-black tree node corresponding to the chunk and then calls chunk_dealloc, that calls chunk_dealloc_mmap and that calls pages_unmap, a wrapper for munmap/VirtualFree. jemalloc neither caches the chunk nor does anything special with it so the behavior is the same.
(Assignee)

Comment 4

6 years ago
(In reply to Nicholas Nethercote [:njn] from comment #2)
> BTW, with the patch applied I couldn't build the browser because
> testGCChunkAlloc.cpp was renamed, but maybe that's a shortcoming of |hg
> diff|.

hg diff does specify the rename in the patch:

diff --git a/js/src/jsapi-tests/testGCChunkAlloc.cpp b/js/src/jsapi-tests/testGCOutOfMemory.cpp
rename from js/src/jsapi-tests/testGCChunkAlloc.cpp
rename to js/src/jsapi-tests/testGCOutOfMemory.cpp
--- a/js/src/jsapi-tests/testGCChunkAlloc.cpp
+++ b/js/src/jsapi-tests/testGCOutOfMemory.cpp

But the patch utility does not understand it.
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d0dd86f41edb

Updated

6 years ago
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/d0dd86f41edb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.