Closed Bug 1276837 Opened 8 years ago Closed 8 years ago

Shrink NodePool::Block to reduce OOM rate

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(2 files, 2 obsolete files)

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.
This struct was last tweaked in bug 1005836.
(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.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch (part 2) - Shrink NodeBlock (obsolete) — Splinter Review
15% of our "small" OOM crashes are allocations of this struct. Halving its size
will hopefully help reduce that.
Attachment #8758107 - Flags: review?(continuation)
Hopefully increased allocation/deallocation doesn't show up too badly in CC times, and also
decreased locality.
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 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+
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.
(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.
I have reordered the patches so I can backport the struct shrinking without
also backporting the renaming.
Attachment #8758107 - Attachment is obsolete: true
Attachment #8758106 - Attachment is obsolete: true
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+
Attachment #8758487 - Flags: review+
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?
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
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)
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)
(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+
I thought Beta uplift was unlikely, but that it didn't hurt to ask :)  Thanks.
You need to log in before you can comment on or make changes to this bug.