Last Comment Bug 702251 - Decommit unused arenas in the background
: Decommit unused arenas in the background
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 670596 714066 714344 714562
Blocks: 707162
  Show dependency treegraph
 
Reported: 2011-11-14 05:54 PST by Igor Bukanov
Modified: 2012-04-17 19:04 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 (11.56 KB, patch)
2011-12-22 00:03 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review
part 2 (13.06 KB, patch)
2011-12-22 00:13 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-11-14 05:54:05 PST
+++ 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 Justin Lebar (not reading bugmail) 2011-11-14 05:57:24 PST
FYI, a simple win that could be had right now is bug 698588.
Comment 2 Terrence Cole [:terrence] 2011-11-14 12:06:19 PST
Also note that we only DecommitFreePages on GC_SHRINK.  These run more frequently now, but still quite rarely.
Comment 3 Igor Bukanov 2011-12-16 06:22:08 PST
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.
Comment 4 Igor Bukanov 2011-12-16 13:58:24 PST
The bug should have not influence on the memory usage, but it should speedup our shrinking GC calls by some milliseconds.
Comment 5 Igor Bukanov 2011-12-22 00:03:55 PST
Created attachment 583732 [details] [diff] [review]
part 1

This preparation patch changes the background sweep code to release the empty GC chunks outside the GC lock.
Comment 6 Igor Bukanov 2011-12-22 00:13:05 PST
Created attachment 583733 [details] [diff] [review]
part 2

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.
Comment 7 Igor Bukanov 2011-12-27 02:27:22 PST
billm - review ping
Comment 8 Bill McCloskey (:billm) 2011-12-27 14:54:00 PST
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?
Comment 9 Igor Bukanov 2011-12-27 15:31:09 PST
(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.
Comment 10 Bill McCloskey (:billm) 2011-12-27 15:59:29 PST
(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 11 Bill McCloskey (:billm) 2011-12-27 16:18:04 PST
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.

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