Closed Bug 1034621 Opened 6 years ago Closed 6 years ago

Make it possible to configure nursery size


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 + fixed
firefox33 --- fixed
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: mccr8, Assigned: jonco)



(Whiteboard: [MemShrink])


(1 file)

As per khuey in bug 1034611 comment 1.
That means that the RIL alone would take up 10% of all the RAM available on a Tarako ...
B2G uses a smaller nursery, IIRC.
Terrence, is there something we can do here like maybe to reduce the worker nursery size or something?  In bug 990355, there's a test that opens 4 workers, and it knocks out the largest contiguous vsize from 125mb to 25mb.

Also, what is the nursery size like on B2G?
Flags: needinfo?(terrence)
Blocks: 990355
Here's a patch to make the nursery size configurable.

Note this has to be done at JSRuntime creation and can't be changed later.

Nursery size is currently fixed at 16MB everywhere.
Attachment #8453756 - Flags: review?(terrence)
Comment on attachment 8453756 [details] [diff] [review]

Review of attachment 8453756 [details] [diff] [review]:


::: js/src/gc/Nursery.h
@@ +201,5 @@
>       */
>      JSRuntime *runtime_;
> +    /* Number of chunks allocated for the nursery. */
> +    int numNurseryChunks_;

It occurs to me that our layout here is really bad. Please move this just below numActiveChunks_ and move currentStart_/currentEnd_ above heapStart_/heapEnd_. This should at least put our hottest members on the same cache line.

::: js/src/jit/BaselineIC.cpp
@@ +5284,5 @@
>      EmitPreBarrier(masm, element, MIRType_Value);
>      masm.storeValue(tmpVal, element);
>      regs.add(key);
> +    if (cx->runtime()->gc.nursery.nurserySize() != 0) {

I think we probably want an API specifically to check for a non-zero-sized nursery e.g. nursery-is-not-permanently-disabled. Let's add Nursery::exists() to make that check and use it here and below.

::: js/src/jit/IonBuilder.cpp
@@ +6545,5 @@
>  jit::NeedsPostBarrier(CompileInfo &info, MDefinition *value)
>  {
> +    return info.executionMode() != ParallelExecution &&
> +        value->mightBeType(MIRType_Object) &&
> +        GetIonContext()->runtime->gcNursery().nurserySize() != 0;


::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4571,5 @@
>      JS_ASSERT(ptr != secondScratchReg_);
>      const Nursery &nursery = GetIonContext()->runtime->gcNursery();
>      uintptr_t startChunk = nursery.start() >> Nursery::ChunkShift;
> +    size_t numNurseryChunks = nursery.nurserySize() >> Nursery::ChunkShift;

Let's add Nursery::numNurseryChunks and do this calculation internally rather than encoding it in the assembler.

::: js/src/jsapi.h
@@ +1263,5 @@
>  JS_ShutDown(void);
>  extern JS_PUBLIC_API(JSRuntime *)
> +JS_NewRuntime(uint32_t maxbytes,
> +              uint32_t maxNurseryBytes = 16 * 1024 * 1024,

We should expose this magic constant -- DefaultNurserySize (ideally in HeapAPI.h) -- so that users that need to pass a parentRuntime aren't stuck guessing.

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +475,5 @@
>    , mLargeAllocationFailureState(OOMState::OK)
>  {
>    mozilla::dom::InitScriptSettings();
> +  mJSRuntime = JS_NewRuntime(aMaxbytes, 16 * 1024 * 1024, aParentRuntime);

Use the DefaultNurserySize constant from HeapAPI.h.
Attachment #8453756 - Flags: review?(terrence) → review+
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Terrence, is there something we can do here like maybe to reduce the worker
> nursery size or something?

Done. :-)
Flags: needinfo?(terrence)
Andrew, does someone from your area want to do the relevant plumbing for the browser side?
Flags: needinfo?(continuation)
Keywords: leave-open
Sure, I can do that.  What size nursery do you think would be appropriate for a worker?
Flags: needinfo?(continuation)
Flags: needinfo?(khuey)
Flags: needinfo?(jcoppeard)
(In reply to Andrew McCreight [:mccr8] from comment #9)

Great!  That's a good question... Off the top of my head I would say go with 4MB to start with and adjust as necessary when we have more data on performance/memory usage.
Flags: needinfo?(jcoppeard)
Perhaps I'm still conflating things here, but a 4 MB nursery will essentially mean that we can't use web workers on b2g.  I think we need the size there to be measured in KB, not MB.
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] ( from comment #11)

Well don't have to use the same size on desktop and b2g.

This size must be a multiple of the chunk size, which is currently 1MB.  We're going to try and reduce this for b2g in bug 941804.
I'll split the browser work off into a new bug.
Component: DOM: Workers → JavaScript: GC
Keywords: leave-open
Summary: We absolutely cannot have a 16 MB nursery for workers → Make it possible to configure nursery size
Whiteboard: [MemShrink]
No longer blocks: 990355, 991845
Assignee: nobody → jcoppeard
Blocks: 1037510
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You should put this up for backporting to Aurora, as it'll be easier to do before it flips over to Beta in a week.  I should be able to get bug 1037510 done before I leave for PTO for a week.  If not, somebody else can finish it off.
Flags: needinfo?(jcoppeard)
Comment on attachment 8453756 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Bug 619558
[User impact if declined]: Workers can take up too much memory.
[Describe test coverage new/current, TBPL]: On central for 4 days.
[Risks and why]: Minor change to add configurability, low risk.
[String/UUID change made/needed]: None
Attachment #8453756 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jcoppeard)
Comment on attachment 8453756 [details] [diff] [review]


FYI, if you want to configure this specifically for B2G 2.0, that will likely need to happen this week.
Attachment #8453756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.