Closed
Bug 702251
Opened 13 years ago
Closed 12 years ago
Decommit unused arenas in the background
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: [Snappy])
Attachments
(2 files)
11.56 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
13.06 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #670596 +++ In the bug 670596 we decommit the arenas on the main thread at the end of the GC sweeping phase. But this implies that we do not decommit arenas that will be released after the GC during the background finalization. Also, this makes our the GC pause longer.
Comment 1•13 years ago
|
||
FYI, a simple win that could be had right now is bug 698588.
Comment 2•13 years ago
|
||
Also note that we only DecommitFreePages on GC_SHRINK. These run more frequently now, but still quite rarely.
Updated•13 years ago
|
Whiteboard: [MemShink]
Assignee | ||
Comment 3•12 years ago
|
||
I will need this for bug 707162. One of options for that bug is to provide explicit API to shrink GC buffers than can be scheduled after the GC. However, even on Linux with fast system calls on a 4-core 3.GHz box decommit may take 20-30 ms when run against 100 MB heap. Doing that in background should avoid the pause.
Blocks: 707162
Assignee | ||
Comment 4•12 years ago
|
||
The bug should have not influence on the memory usage, but it should speedup our shrinking GC calls by some milliseconds.
Whiteboard: [MemShink] → [Snappy]
Assignee | ||
Comment 5•12 years ago
|
||
This preparation patch changes the background sweep code to release the empty GC chunks outside the GC lock.
Attachment #583732 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 6•12 years ago
|
||
Here is an implementation of the background decommit. To minimize the amount of time the GC lock is taken the patch decommit each arena outside the GC lock. To ensure the GC structures consistency the patch set aside the free arena as if it was allocated, then releases the lock, decommits it, takes the lock again and only then mark the arena as decommited. The fallible decommit adds some annoyances, but they are not grave to assume infallibility. Another change is that the patch decommits the arenas from the tail of the chunk list. This minimized the interference with the main thread in case the the latter starts new allocations.
Attachment #583733 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 7•12 years ago
|
||
billm - review ping
Comment on attachment 583732 [details] [diff] [review] part 1 Review of attachment 583732 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/jsgc.cpp @@ +552,5 @@ > + FreeChunk(chunk); > +} > + > +/* static */ inline void > +Chunk::releaseList(Chunk *chunkListHead) This seems to make more sense as a normal static function, rather than as a static member of Chunk. Then we don't have to expose it outside of jsgc.cpp. @@ +564,5 @@ > +inline void > +Chunk::updateReleaseCounters(JSRuntime *rt) > +{ > + JS_ASSERT(rt->gcNumArenasFreeCommitted >= info.numArenasFreeCommitted); > + rt->gcNumArenasFreeCommitted -= info.numArenasFreeCommitted; Would it be possible to set info.numArenasFreeCommitted to 0 after this, and then to assert that it's 0 in releaseList? That might avoid problems where we forget to call updateReleaseCounters. @@ -2378,5 @@ > - /* > - * Expire the chunks released during the GC so they will be available to > - * the rest of the system immediately. > - */ > - rt->gcChunkPool.expire(rt, shouldShrink()); Could we still expire before background sweeping, and then do it again afterward?
Attachment #583732 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #8) > @@ -2378,5 @@ > > - /* > > - * Expire the chunks released during the GC so they will be available to > > - * the rest of the system immediately. > > - */ > > - rt->gcChunkPool.expire(rt, shouldShrink()); > > Could we still expire before background sweeping, and then do it again > afterward? This should not matter for two reasons. First this will delay the running of finalizers that may also release memory via free calls etc. Second, if we managed to schedule the shrinking GC at the right time, then the main thread would not be running and then it does not matter if we release first the empty chunks or malloc memory first. But if the main thread has started to do allocations, then it is better to delay slightly the empty chunks release so they can be grabbed for allocations. So in the patch I opted for simpler code and release the empty chunks only once after finalization.
(In reply to Igor Bukanov from comment #9) > So in the patch I opted for simpler code and release the empty chunks only > once after finalization. That sounds fine.
Comment on attachment 583733 [details] [diff] [review] part 2 Review of attachment 583733 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks. ::: js/src/jsgc.cpp @@ +2259,5 @@ > + break; > + > + /* > + * prevp exists and is not the list head. It must point to the next > + * field os the previous chunk. os -> of @@ +2263,5 @@ > + * field os the previous chunk. > + */ > + uintptr_t prevAddress = reinterpret_cast<uintptr_t>(chunk->info.prevp); > + JS_ASSERT((prevAddress & ChunkMask) == offsetof(Chunk, info.next)); > + chunk = reinterpret_cast<Chunk *>(prevAddress - offsetof(Chunk, info.next)); Let's add a Chunk method for this.
Attachment #583733 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7884cb42b9de https://hg.mozilla.org/integration/mozilla-inbound/rev/e76919e51dc3
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7884cb42b9de https://hg.mozilla.org/mozilla-central/rev/e76919e51dc3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•