Closed Bug 875435 Opened 8 years ago Closed 8 years ago

Allow the Nursery to grow and shrink with demand

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

I've split the patch up into three parts to simplify review:

The first part counts the size of things we tenure. Note, it would be awesome if we could just use rt->gcBytes, but I think we also really need to count slot/element data here to get a useful picture of our working-set-size.
Attachment #753395 - Flags: review?(wmccloskey)
Attached patch 2 of 3: v0Splinter Review
This patch changes NurseryLayout/asLayout to NurseryChunkLayout/asChunk(i) and uses the chunk concept throughout the Nursery.
Attachment #753401 - Flags: review?(wmccloskey)
Attached patch 3 of 3: v0Splinter Review
This patch implements the actual growing and shrinking heuristics and logic.
Attachment #753402 - Flags: review?(wmccloskey)
Blocks: 875863
No longer blocks: 706885
Attachment #753395 - Flags: review?(wmccloskey) → review?(bhackett1024)
Attachment #753401 - Flags: review?(wmccloskey) → review?(bhackett1024)
Comment on attachment 753402 [details] [diff] [review]
3 of 3: v0

Bill is on vacation, so switching review to Brian.
Attachment #753402 - Flags: review?(wmccloskey) → review?(bhackett1024)
Comment on attachment 753395 [details] [diff] [review]
1 of 3: collect tenured size

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

Sorry for the delay.
Attachment #753395 - Flags: review?(bhackett1024) → review+
Comment on attachment 753401 [details] [diff] [review]
2 of 3: v0

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

::: js/src/gc/Nursery.h
@@ +103,5 @@
> +    /* Pointer to the last byte of space in the current chunk. */
> +    uintptr_t currentEnd_;
> +
> +    /* The chunk that is currently being allocated from. */
> +    int currentChunk_;

Maybe 'The index of the chunk that is ...'

@@ +106,5 @@
> +    /* The chunk that is currently being allocated from. */
> +    int currentChunk_;
> +
> +    /* The index after the last chunk that we will allocate from. */
> +    int endChunk_;

Could this variable be numActiveChunks_ instead, for a more obvious meaning?

@@ +146,2 @@
>      };
> +    NurseryChunkLayout &asChunk(int offset) const {

The 'as' in this method name is a bit troublesome, as it suggests a downcast rather than a plain accessor.  Maybe just Nursery::chunk(i) instead?  Also, the |offset| variable should be |index| instead.

@@ +164,5 @@
> +    JS_ALWAYS_INLINE void setCurrentChunk(int chunkno) {
> +        JS_ASSERT(chunkno < NumNurseryChunks);
> +        JS_ASSERT(chunkno < endChunk_);
> +        currentChunk_ = chunkno;
> +        currentEnd_ = asChunk(chunkno).end();

It looks like this method could also update position_, which would allow removing writes to position_ in the callers.
Attachment #753401 - Flags: review?(bhackett1024) → review+
Comment on attachment 753402 [details] [diff] [review]
3 of 3: v0

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

::: js/src/gc/Nursery.cpp
@@ +520,5 @@
>          JSObject *obj = static_cast<JSObject*>(p->forwardingAddress());
>          JS_TraceChildren(&trc, obj, MapAllocToTraceKind(obj->tenuredGetAllocKind()));
>      }
>  
> +    /* Resize the nursery to closer match our current working set size. */

grammar
Attachment #753402 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/11dcc05b46a0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 1507445
You need to log in before you can comment on or make changes to this bug.