Last Comment Bug 686144 - Eliminate gc::MarkingDelay
: Eliminate gc::MarkingDelay
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: 686017
Blocks: 671702
  Show dependency treegraph
 
Reported: 2011-09-10 11:08 PDT by Igor Bukanov
Modified: 2011-09-20 07:46 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (18.02 KB, patch)
2011-09-10 11:11 PDT, Igor Bukanov
wmccloskey: review+
Details | Diff | Review

Description Igor Bukanov 2011-09-10 11:08:21 PDT
To implement delayed marking, that we use when the GC marking requirements exceeds the capacity of the normal marking stack, we use one extra word per GC arena. The word allows to link the arena with things that caused marking delay. However, due to layout inefficiencies the real cost with 1Mb GC chunks is about 0.4% and it grows to 1.6% if we change the GC chunk size to 256K, which is one of the options I consider for the bug 671702 to decrease our heap fragmentation. 

For example, with 256K chunks without the Chunk::markingDelays arrays we can fit 63 arenas into the chunk. 63 arenas and their marking bitmaps takes 256K - 64 bytes and those 64 bytes is enough to fit ChunkInfo. But it is not enough to also fit the array of 63 marking delay words. That results in only 62 available arenas or 1.6% overhead causing noticeable regression in v8 benchmark.

To avoid that we can pack the delayed marking linkage together with ArenaHead::allocKind. The arena pointers always has ArenaShift (12 currently) low bits cleared and that is enough to fit ArenaHeader::allocKind.

The bug depends on the 686017 bug as it would make access to ArenaHeader::allocKind slightly slower and without that bug it causes visible V8 regression.
Comment 1 Igor Bukanov 2011-09-10 11:11:34 PDT
Created attachment 559708 [details] [diff] [review]
v1

The patch eliminates separated Chunk::markingDelay array and merges its words with the arena header.
Comment 2 Bill McCloskey (:billm) 2011-09-19 13:07:21 PDT
Comment on attachment 559708 [details] [diff] [review]
v1

Review of attachment 559708 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsgc.h
@@ +386,5 @@
> +     * stack.
> +     *
> +     * To minimize the ArenaHeader size we record the next delayed marking
> +     * linkage as arenaAddress() >> ArenaShift and pack it with the allocKind
> +     * field and hasDelayedMarking flag. We use 8 bites for the allocKind, not

bites->bits

@@ +392,5 @@
> +     * to access it.
> +     */
> +  public:
> +    size_t       hasDelayedMarking  : 1;
> +    size_t       nextDelayedMarking : JS_BITS_PER_WORD - 8 - 1;

Could you add a getter and a setter for nextDelayedMarking that do the bit shifts and casting?
Comment 3 Igor Bukanov 2011-09-19 15:12:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/104fb6df714f - landed with getter and setter change
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:46:24 PDT
https://hg.mozilla.org/mozilla-central/rev/104fb6df714f

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