Closed
Bug 1276837
Opened 8 years ago
Closed 8 years ago
Shrink NodePool::Block to reduce OOM rate
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(2 files, 2 obsolete files)
1.83 KB,
patch
|
n.nethercote
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
13.74 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
Here's a search of crash-stats for small OOMs in the past week: https://crash-stats.mozilla.com/search/?signature=~OOM%20%7C%20small&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature Here's the same search, but with an extra search term: "OOM Allocation Size" = 163824: https://crash-stats.mozilla.com/search/?signature=~OOM%20%7C%20small&oom_allocation_size=163824&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature At the time of writing, the first search gives 101,518 hits and the second gives 15,473 hits. 15% of our small OOMs are 163824 bytes, which is the size of NodePool::Block. That's a lot! It's a shame to OOM during cycle collection which is trying to free memory. If we reduce the size by 2x or 4x it'll be substantially less likely to OOM while hopefully affecting speed negligibly.
Assignee | ||
Comment 1•8 years ago
|
||
This struct was last tweaked in bug 1005836.
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > This struct was last tweaked in bug 1005836. But more substantially in bug 658672, which shrank the size by 4x.
Assignee | ||
Comment 3•8 years ago
|
||
nsCycleCollector.cpp has three different structs named "Block", which makes it hard to read. This patch renames them as EdgeBlock, NodeBlock, and PurpleBlock.
Attachment #8758106 -
Flags: review?(continuation)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
15% of our "small" OOM crashes are allocations of this struct. Halving its size will hopefully help reduce that.
Attachment #8758107 -
Flags: review?(continuation)
Comment 5•8 years ago
|
||
Hopefully increased allocation/deallocation doesn't show up too badly in CC times, and also decreased locality.
Comment 6•8 years ago
|
||
Comment on attachment 8758106 [details] [diff] [review] (part 1) - Rename the CC Block structs Review of attachment 8758106 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this has always been annoying. You could have also explicitly qualified their names (eg EdgePool::Block) but I guess this is okay.
Attachment #8758106 -
Flags: review?(continuation) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8758107 [details] [diff] [review] (part 2) - Shrink NodeBlock Review of attachment 8758107 [details] [diff] [review]: ----------------------------------------------------------------- Worth a shot.
Attachment #8758107 -
Flags: review?(continuation) → review+
Comment 8•8 years ago
|
||
Also, in case you haven't noticed it, if you add "proto signature" as a facet to your OOM | small search, you can see the precise breakdown of OOM | smalls.
Comment 9•8 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5) > Hopefully increased allocation/deallocation doesn't show up too badly in CC > times, and also > decreased locality. The blocks are still huge. It shouldn't matter much.
Assignee | ||
Comment 10•8 years ago
|
||
I have reordered the patches so I can backport the struct shrinking without also backporting the renaming.
Assignee | ||
Updated•8 years ago
|
Attachment #8758107 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8758106 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8758486 [details] [diff] [review] (part 1) - Shrink NodePool::Block Review of attachment 8758486 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over r+.
Attachment #8758486 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8758487 -
Flags: review+
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75d948bafca38c5ce9ed9c79e3b385136ddf1abc Bug 1276837 (part 1) - Shrink NodePool::Block. r=mccr8. https://hg.mozilla.org/integration/mozilla-inbound/rev/a584b40f1933d36ca0ca22de516b17e481761b1a Bug 1276837 (part 2) - Rename the CC Block structs. r=mccr8.
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8758486 [details] [diff] [review] (part 1) - Shrink NodePool::Block Approval Request Comment [Feature/regressing bug #]: N/A. [User impact if declined]: 15% of our "OOM | small" crashes are due to the cycle collector's NodePool::Block struct. This patch halves its size (from 160 KiB to 80 KiB on 32-bit) which will hopefully reduce the OOM rate, which would let cycle collection complete, which will hopefully make more memory available again. [Describe test coverage new/current, TreeHerder]: cycle collection is frequently exercised during any browser activity. [Risks and why]: negligible. Patch just changes the size of a buffer. [String/UUID change made/needed]: none.
Attachment #8758486 -
Flags: approval-mozilla-beta?
Attachment #8758486 -
Flags: approval-mozilla-aurora?
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75d948bafca3 https://hg.mozilla.org/mozilla-central/rev/a584b40f1933
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Hi Andrew, Jim: Could you please give me a second opinion on this uplift for 47? I will gtb RC2 tomorrow which has only one gfx crash fix in it. I am on the fence for this fix because we do not have any verification from Nightly/Aurora to tell us whether this is helping or not. What do you think? Is this worth uplifting in RC2? Note: Any critical regressions would cause a delay in Fx47 release at this point.
Flags: needinfo?(jmathies)
Flags: needinfo?(continuation)
Comment 17•8 years ago
|
||
Yeah, I don't think this is worth uplifting to 47 at this point. I'm not sure how much it will really help.
Flags: needinfo?(continuation)
Comment 18•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #17) > Yeah, I don't think this is worth uplifting to 47 at this point. I'm not > sure how much it will really help. agreed.
Flags: needinfo?(jmathies)
Thanks Andrew and Jim. It is too late to take this in Fx47 especially because the fix hasn't been verified/baked on Nightly and Aurora.
Comment on attachment 8758486 [details] [diff] [review] (part 1) - Shrink NodePool::Block Please see my previous comment. Let's uplift to Aurora48.
Attachment #8758486 -
Flags: approval-mozilla-beta?
Attachment #8758486 -
Flags: approval-mozilla-beta-
Attachment #8758486 -
Flags: approval-mozilla-aurora?
Attachment #8758486 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 21•8 years ago
|
||
I thought Beta uplift was unlikely, but that it didn't hurt to ask :) Thanks.
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/014467d4ca30
You need to log in
before you can comment on or make changes to this bug.
Description
•