Closed
Bug 652781
Opened 13 years ago
Closed 11 years ago
Investigate if cycle collector could unroot asynchronously
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: smaug, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Snappy:P2])
Attachments
(2 files, 3 obsolete files)
5.92 KB,
patch
|
Details | Diff | Splinter Review | |
5.08 KB,
patch
|
Details | Diff | Splinter Review |
To reduce long pauses, I wonder if we could first traverse and collect objects to unlink, and then post a timer to do the actual unlink in a few seconds. Unlinking and deleting for example dom nodes is not very fast.
Reporter | ||
Comment 1•13 years ago
|
||
Need to evaluate this.
Comment 2•13 years ago
|
||
Does it need to be a timer? Posting to the event loop might be enough to respond to a user interaction.
Reporter | ||
Comment 3•13 years ago
|
||
posting to event loop happens probably too soon.
Reporter | ||
Comment 4•13 years ago
|
||
er, I mean handling of the runnable in the event loop happens probably too soon.
Reporter | ||
Comment 5•13 years ago
|
||
This is anyway using runnables. The patch is hackish, but something I wanted to try out. It splits xpcom object unlink to batches of 512 releases.
Comment 6•13 years ago
|
||
This sounds like it is worth investigating. When looking at CC times on my own, unlinking is the only thing that ever takes much time aside from graph building. Peter mentioned some kind of caching table of weak pointers that can decide that a weak pointer it holds should become strong. At least, that was my understanding. It seems like that would doom any effort to allow the browser to run in between computing dead objects and freeing them. Any time an object is revived this way, the entire set of dead objects has to be invalidated because any of them could have become live again. Furthermore, the set of dead objects can't just be thrown away because it is possible that the garbage includes a real dead cycle that we'll leak. That can be solved either by pausing the browser again and running a special mini-CC just on the garbage we found or by pushing all of the garbage back into the purple buffer.
Reporter | ||
Comment 7•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=50f9c505aa80
Reporter | ||
Updated•13 years ago
|
Summary: Investigate if cycle collector could unlink asynchronously → Investigate if cycle collector could unroot asynchronously
Reporter | ||
Comment 8•13 years ago
|
||
So, in some cases, like when closing a large, non-js-heavy web page, unrooting nsISupports objects can take almost 30% of the CC time on an opt linux build. Unfortunately tryserver shows that the patch has major problems. I'll investigate what and why.
Reporter | ||
Comment 9•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #8) > So, in some cases, like when closing a large, non-js-heavy web page, > unrooting nsISupports objects can > take almost 30% of the CC time on an opt linux build. Apparently not always only non-js-heavy. When I closed a tab which had Facebook loaded, releasing/unrooting nsISupports objects took 16ms.
Reporter | ||
Comment 10•13 years ago
|
||
Adds a null checks. https://tbpl.mozilla.org/?tree=Try&rev=31d55cb3ec1c
Comment 11•13 years ago
|
||
Interesting. So I guess CollectWhite time is mostly taken up by Unroot?
Reporter | ||
Comment 12•13 years ago
|
||
The previous one doesn't pass on debug builds. Changing the assertion check a bit, hopefully the crash is about that. I didn't yet look at what could have caused that ::PreCreate problem. Something to do with XBL, I think.
Attachment #572831 -
Attachment is obsolete: true
Attachment #573177 -
Attachment is obsolete: true
Reporter | ||
Comment 13•13 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #11) > Interesting. So I guess CollectWhite time is mostly taken up by Unroot? I think it depends on the case. Releasing and possibly deleting for example DOM nodes is quite expensive, but unrooting some other objects can be a lot faster.
Reporter | ||
Comment 14•13 years ago
|
||
The latest patch looks ok on try https://hg.mozilla.org/try/rev/6dbdb8fb69db, well, at least so far on linux. But I don't yet quite understand the ::PreCreate assertion problem. How the old code can guarantee that there is always outerwindow...
Reporter | ||
Comment 15•13 years ago
|
||
Some random timing info (without the patch) (Closed the following tabs: FB (not logged in), GMail, CNN) root time 1ms unlink time 15ms unroot time 4ms (Closed the following tabs: D3E, DOM4) root time 5ms unlink time 30ms unroot time 18ms (Closed the following tab: HTML spec) root time 13ms unlink time 100ms unroot time 144ms
Reporter | ||
Comment 16•13 years ago
|
||
Looks like there are some leaks, but nothing terribly bad. I'll investigate some more.
Reporter | ||
Comment 17•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=afef7a4f1222
Reporter | ||
Comment 18•13 years ago
|
||
Attachment #573193 -
Attachment is obsolete: true
Updated•13 years ago
|
Hardware: x86_64 → All
Version: unspecified → Trunk
Updated•13 years ago
|
Whiteboard: [Snappy]
Comment 19•12 years ago
|
||
Andrew, can you snappy-prioritize this?
Comment 20•12 years ago
|
||
Marking P2. Right now, the CollectWhite phase mostly takes no time, except when you are freeing a bunch of stuff and it can take 20% or more of the CC time, and this optimization will let us split out the frees from that part of it. I'm not sure how much of the time it is, but probably a fair amount.
Whiteboard: [Snappy] → [Snappy:P2]
Reporter | ||
Comment 21•12 years ago
|
||
Uploaded to try to see how this behaves now. https://tbpl.mozilla.org/?tree=Try&rev=e3c563c3bee0
Comment 22•12 years ago
|
||
Linux M1 leaked 1.5 megs, which sounds like it could be due to this patch.
Reporter | ||
Comment 23•12 years ago
|
||
Very possible
Reporter | ||
Comment 24•12 years ago
|
||
Looks like there is only 1 document leaked. Shouldn't be too hard to find the leak.
Reporter | ||
Comment 25•11 years ago
|
||
CC has changed significantly since filing this. WONTFIX.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•