Last Comment Bug 684569 - Remove custom chunk allocation
: Remove custom chunk allocation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 671702
  Show dependency treegraph
 
Reported: 2011-09-04 03:29 PDT by Igor Bukanov
Modified: 2011-09-07 09:00 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (12.32 KB, patch)
2011-09-04 03:34 PDT, Igor Bukanov
n.nethercote: review+
Details | Diff | Review

Description Igor Bukanov 2011-09-04 03:29:33 PDT
+++ 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 2011-09-04 03:34:38 PDT
Created attachment 558133 [details] [diff] [review]
v1

The patch removes js::GCChunkAllocator and its users and turns jsapi-test/testGCChunkAlloc into OOM checker.
Comment 2 Nicholas Nethercote [:njn] 2011-09-04 23:45:54 PDT
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 2011-09-05 00:25:28 PDT
(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 2011-09-05 00:30:57 PDT
(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 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-07 07:13:42 PDT
http://hg.mozilla.org/mozilla-central/rev/d0dd86f41edb

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