Closed Bug 1101921 Opened 10 years ago Closed 9 years ago

GGC nursery size is too large on B2G

Categories

(Core :: JavaScript: GC, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: erahm, Assigned: terrence)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

From discussion on IRC with Terrence this should be 4MB, but it looks like it's ending up 16MB.

A memory report brought this to my attention:
│  │  │  ├──11.75 MB (18.76%) ── nursery-committed
So based on bug 1020751 and bug 988369 I thought it was somewhere between 256 and 64K?  How did it get to be 16 MB ?!?
DefaultNurseryBytes was getting set to 16MiB unconditionally. It should be 16 * ChunkSize, so that it will scale down nicely for B2G.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8525623 - Flags: review?(jcoppeard)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> So based on bug 1020751 and bug 988369 I thought it was somewhere between
> 256 and 64K?  How did it get to be 16 MB ?!?

We fixed the default ChunkSize to be 256K... but apparently forgot to make the maximum scale with that size. D'oh!
Comment on attachment 8525623 [details] [diff] [review]
fix_nursery_size_on_b2g-v0.diff

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

Bug 1034621 made this configurable but left the default at 16MB.  I thought the idea was that B2G would then choose a different size, but I guess that didn't happen.

Making the default a function of the chunk size makes sense though.
Attachment #8525623 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8525623 [details] [diff] [review]
> fix_nursery_size_on_b2g-v0.diff
> 
> Review of attachment 8525623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bug 1034621 made this configurable but left the default at 16MB.  I thought
> the idea was that B2G would then choose a different size, but I guess that
> didn't happen.
> 
> Making the default a function of the chunk size makes sense though.

Yeah, it's a simple enough heuristic for a better default behavior and B2G can still tweak it as needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/012e11e54840
https://hg.mozilla.org/mozilla-central/rev/281d52f5d0e6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8525623 - Flags: approval-mozilla-b2g34?
:erahm, please fill in the approval request form to consider the b2g34 approval. My main question here is risk and how safe this change is giving we are taking only ship-blockers on 2.1 and this could just ride the trains? Will the lower threshold that we are trying to set trigger more OOM's etc?
Flags: needinfo?(erahm)
Comment on attachment 8525623 [details] [diff] [review]
fix_nursery_size_on_b2g-v0.diff

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1020751 I guess. We were supposed to have a smaller nursery size on b2g. 
User impact if declined: Higher risk of OOM. This is a 12MiB regression per app.
Testing completed: Landed on central, nothing broke.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Flags: needinfo?(erahm) → needinfo?(bbajaj)
(In reply to Eric Rahm [:erahm] from comment #9)
> Comment on attachment 8525623 [details] [diff] [review]
> fix_nursery_size_on_b2g-v0.diff
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 1020751 I guess. We were
> supposed to have a smaller nursery size on b2g. 
> User impact if declined: Higher risk of OOM. This is a 12MiB regression per
> app.
> Testing completed: Landed on central, nothing broke.

At this point , I need more testing that m-c. Can you help test this on a real device with 2.2 to get more confidence and provide more data here ?
> Risk to taking this patch (and alternatives if risky): Low.
> String or UUID changes made by this patch: None.
Flags: needinfo?(bbajaj)
I'll do some ad-hoc testing w/ m-c on b2g.
Flags: needinfo?(erahm)
I tested on my Flame w/ the latest nightly and observed no issues (browser, camera, etc). Additionally I can confirm that memory used for my test app in the nursery-committed category is now below 4MB.

:bajaj is this enough testing for you?
Flags: needinfo?(erahm) → needinfo?(bbajaj)
(In reply to Eric Rahm [:erahm] from comment #12)
> I tested on my Flame w/ the latest nightly and observed no issues (browser,
> camera, etc). Additionally I can confirm that memory used for my test app in
> the nursery-committed category is now below 4MB.
> 
> :bajaj is this enough testing for you?

Thanks for helping with the on-device testing here Eric. Given the memory win here, lets lets take this on 2.1. Please let QA know(add verifyme and sample testing scenarios ?) if you want them to do more testing to memory intensive apps/game etc to get more confidence here.
Flags: needinfo?(bbajaj)
Attachment #8525623 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
You need to log in before you can comment on or make changes to this bug.