Andrew McCreight [:mccr8] Reporter
	Description • 10 years ago

As per khuey in bug 1034611 comment 1.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 1 • 10 years ago

That means that the RIL alone would take up 10% of all the RAM available on a Tarako ...

	Andrew McCreight [:mccr8] Reporter
	Comment 2 • 10 years ago

B2G uses a smaller nursery, IIRC.

	Andrew McCreight [:mccr8] Reporter
	Comment 3 • 10 years ago

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?

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 4 • 10 years ago

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.

	Terrence Cole [:terrence]
	Comment 5 • 10 years ago

Comment on attachment 8453756 [details] [diff] [review]
make-nursery-size-configurable

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

Excellent!

::: 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);
>  #ifdef JSGC_GENERATIONAL
> +    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;

Nursery::exists()

::: 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.

	Terrence Cole [:terrence]
	Comment 6 • 10 years ago

(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. :-)

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/037da8d9b3c9

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 8 • 10 years ago

Andrew, does someone from your area want to do the relevant plumbing for the browser side?

	Andrew McCreight [:mccr8] Reporter
	Comment 9 • 10 years ago

Sure, I can do that.  What size nursery do you think would be appropriate for a worker?

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 10 • 10 years ago

(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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 11 • 10 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 12 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.

	Andrew McCreight [:mccr8] Reporter
	Comment 13 • 10 years ago

I'll split the browser work off into a new bug.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 14 • 10 years ago

Ok, let's do 1 MB for now.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/037da8d9b3c9

	Andrew McCreight [:mccr8] Reporter
	Comment 16 • 10 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 17 • 10 years ago

Comment on attachment 8453756 [details] [diff] [review]
make-nursery-size-configurable

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 18 • 10 years ago

Comment on attachment 8453756 [details] [diff] [review]
make-nursery-size-configurable

Aurora+

FYI, if you want to configure this specifically for B2G 2.0, that will likely need to happen this week.

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 19 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f0b51d12caf