Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[CC] Don't add JS holders with no gray children as XPConnect roots

RESOLVED FIXED in mozilla14

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
These aren't a huge deal, but I'm seeing about 500 to 1100 nsXULPrototypes in each CC graph.  Jonas said these probably live for the duration of the browser, so maybe we can avoid visiting them in the cycle collector except at shutdown, or not cycle collect them altogether and add some special shutdown code to remove them.
I would guess that they'll simply go away when we shut down since they would be no longer held on to by the xul-cache or any live documents. I.e. I suspect their refcount will go down to 0 and we'll just kill them the "normal" way.
(Assignee)

Updated

6 years ago
Blocks: 716598
(Assignee)

Updated

6 years ago
No longer blocks: 698919
An alternative, and safer, way to deal with them is to keep their CC code, but make the CC code bail if called before we have started shutting down.

This way our shut-down behavior is exactly as it is now and we don't risk any cycles keeping things alive there.
(Assignee)

Comment 3

6 years ago
Yeah, that's true.  We can even leave things like that for a few release cycles, and later cut out the CC code if nothing seems to show up.  I also need to look into how much of this is actually traversed after bug 713865.
(Assignee)

Comment 4

6 years ago
A bunch of nsXULPrototypeNodes still show up in the CC graph even with Smaug's patches, so this may still be worth investigating.
(Assignee)

Updated

5 years ago
Blocks: 722715
(Assignee)

Comment 5

5 years ago
It looks like nsXULPrototypeNodes are being added to the CC graph because they are JS holders, even if the JS they contain only pointers to black JS.  They are not nsISupports, so they are never in the purple buffer.  I think we should be able to fix this (and perhaps other things) by changing CheckParticipatesInCycleCollection, in XPCJSRuntime.  I don't 100% understand why that function is the way it is, so I'll have to talk to Peter about it.
Assignee: nobody → continuation
Summary: investigate not cycle collecting nsXULPrototype nodes → avoid adding nsXULPrototype nodes to the CC graph
(Assignee)

Updated

5 years ago
Component: XUL → XPConnect
QA Contact: xptoolkit.widgets → xpconnect
Summary: avoid adding nsXULPrototype nodes to the CC graph → [CC] Don't add JS holders with no gray children as XPConnect roots
(Assignee)

Comment 6

5 years ago
Created attachment 609869 [details] [diff] [review]
add less JS holders as roots

I talked to Peter, and he can't see why this only checks the last object a JS holder holds, so I switched it back to being a flag that stays on once it is on.  It starts off, and is set to true only if we see a gray JS child.  This gets rid of the bulk of nsXULPrototypeNodes from the graph.

There are a few odd cases where they hold onto nsNodeInfo (XBL) or some JS, but we can handle that in a followup.
(Assignee)

Comment 7

5 years ago
If a JS holder is in the purple buffer, it will still be added later, but that's okay.
(Assignee)

Comment 8

5 years ago
By "that's okay", I mean "that's necessary for correctness".
(Assignee)

Comment 9

5 years ago
Comment on attachment 609869 [details] [diff] [review]
add less JS holders as roots

Try run came back looking good.

https://tbpl.mozilla.org/?tree=Try&rev=f7ec71615b11

And before you complain, these weird ifs are in proper Spidermonkey style. ;)

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Conditionals
Attachment #609869 - Flags: review?(bugs)

Updated

5 years ago
Attachment #609869 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e919664cdf5
Target Milestone: --- → mozilla14
(Assignee)

Updated

5 years ago
Whiteboard: [Snappy]
https://hg.mozilla.org/mozilla-central/rev/0e919664cdf5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.