Closed
Bug 1101921
Opened 10 years ago
Closed 9 years ago
GGC nursery size is too large on B2G
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: erahm, Assigned: terrence)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
1.65 KB,
patch
|
jonco
:
review+
bajaj
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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 ?!?
Assignee | ||
Comment 2•10 years ago
|
||
DefaultNurseryBytes was getting set to 16MiB unconditionally. It should be 16 * ChunkSize, so that it will scale down nicely for B2G.
Assignee | ||
Comment 3•10 years ago
|
||
(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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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
Assignee | ||
Comment 6•10 years ago
|
||
I am dumb. Backed out and relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf715632ae0 https://hg.mozilla.org/integration/mozilla-inbound/rev/281d52f5d0e6
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/281d52f5d0e6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter | ||
Updated•9 years ago
|
Attachment #8525623 -
Flags: approval-mozilla-b2g34?
Comment 8•9 years ago
|
||
: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)
Reporter | ||
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
(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)
Reporter | ||
Comment 12•9 years ago
|
||
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)
Comment 13•9 years ago
|
||
(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)
Updated•9 years ago
|
Attachment #8525623 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/29ba3cc74d0a
status-b2g-v2.1:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-firefox34:
--- → wontfix
status-firefox35:
--- → wontfix
status-firefox36:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•