Closed Bug 702251 Opened 13 years ago Closed 12 years ago

Decommit unused arenas in the background

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: [Snappy])

Attachments

(2 files)

+++ 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.
FYI, a simple win that could be had right now is bug 698588.
Also note that we only DecommitFreePages on GC_SHRINK.  These run more frequently now, but still quite rarely.
Whiteboard: [MemShink]
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
The bug should have not influence on the memory usage, but it should speedup our shrinking GC calls by some milliseconds.
Whiteboard: [MemShink] → [Snappy]
Attached patch part 1Splinter Review
This preparation patch changes the background sweep code to release the empty GC chunks outside the GC lock.
Attachment #583732 - Flags: review?(wmccloskey)
Attached patch part 2Splinter Review
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)
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+
(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+
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
Depends on: 714066
Depends on: 714280
No longer depends on: 714280
Depends on: 714344
Depends on: 714562
You need to log in before you can comment on or make changes to this bug.