684569 - Remove custom chunk allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ 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.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch removes js::GCChunkAllocator and its users and turns jsapi-test/testGCChunkAlloc into OOM checker.

Comment 2

[Nicholas Nethercote [inactive]]

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|.

Comment 3

[Igor Bukanov]

(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.

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

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

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d0dd86f41edb