Closed
Bug 1096624
Opened 10 years ago
Closed 10 years ago
Use a SegmentedArray in nsCycleCollector::CollectWhite
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: away, Assigned: n.nethercote)
References
Details
(Keywords: crash, Whiteboard: [MemShrink:P2])
Crash Data
Attachments
(2 files)
3.04 KB,
text/plain
|
Details | |
7.08 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-f0d21639-644d-4036-ae6e-fe6f32141103. ============================================================= The array in CollectWhite is about 1% of release channel OOMs. It looks like a great candidate for a SegmentedArray.
Comment 1•10 years ago
|
||
In IRC, dmajor said about the sizes of these OOM crashes, "roughly 40% between 1-2mb, another 40% between 2-3", "ah so the next 40% is 2-10mb", so the array is getting quite giant. dmajor also said "My goal is just to make sure we can survive when we have only 1mb 'holes' in the address space available". I think we gather telemetry on the values of this in CYCLE_COLLECTOR_COLLECTED. As you can see, mostly they are very small, so it would be nice to not use a ton of memory when we don't have to. The CC also runs per-worker. The telemetry there is CYCLE_COLLECTOR_WORKER_COLLECTED. The current implementation is |nsAutoTArray<PtrInfo*, 4000>|. All we do with this array is push things on it, then iterate over it a few times, calling methods on it.
![]() |
Assignee | |
Comment 2•10 years ago
|
||
I can take this. It's similar to bug 1094564.
Assignee: dmajor → n.nethercote
![]() |
Assignee | |
Comment 3•10 years ago
|
||
Here's some data from running http://gregor-wagner.com/tmp/mem50. Each lines shows the length and capacity. Almost all of them are less than 10% full.
Comment 4•10 years ago
|
||
I recently changed this so we only put in C++ objects, not JS ones (which we just run no-ops on), which is probably why they are often very empty now.
![]() |
Assignee | |
Comment 5•10 years ago
|
||
This patch does the following. - Moves the logic for computing the ideal capacity for a SegmentedArray out of SnowWhiteKiller into its own class, SegmentedArrayCapacity. - Adds SegmentedArray::Length(). - Replaces the nsTArray in CollectWhite(), which can be very large, with a SegmentedArray.
Attachment #8520526 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 6•10 years ago
|
||
Try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=20c19fd2e1f2
Comment 7•10 years ago
|
||
Comment on attachment 8520526 [details] [diff] [review] Use a SegmentedArray in nsCycleCollector::CollectWhite Don't understand '// njn: resize' comment. Drop it or change it to something better. I would just not add the Length() and move uint32_t numWhiteNodes; to be above uint32_t numWhiteGCed = 0; (and initialize numWhiteNodes to 0) and then ++numWhiteNodes whenever we do whiteNodes.AppendElement(pinfo); With those, r+.
Attachment #8520526 -
Flags: review?(bugs) → review+
![]() |
Assignee | |
Comment 8•10 years ago
|
||
> Don't understand '// njn: resize' comment. > Drop it or change it to something better. Whoops. I write comments like that as notes to myself for things that need fixing before review or landing. I forgot to remove this one. > I would just not add the Length() and > move uint32_t numWhiteNodes; to be above uint32_t numWhiteGCed = 0; > (and initialize numWhiteNodes to 0) > and then ++numWhiteNodes whenever we do whiteNodes.AppendElement(pinfo); Will do. Thank you for the quick review.
![]() |
Assignee | |
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
![]() |
Assignee | |
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/91e6e73aa5e6
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/91e6e73aa5e6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
![]() |
Assignee | |
Updated•10 years ago
|
Depends on: cumulative-heap-profiling
You need to log in
before you can comment on or make changes to this bug.
Description
•