[CC] Mark JS objects reachable from obviously live C++ objects black during GC marking

RESOLVED INCOMPLETE

Status

()

Core
JavaScript Engine
RESOLVED INCOMPLETE
7 years ago
10 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

299 bytes, text/html
Details
7.59 KB, patch
Details | Diff | Splinter Review
5.46 KB, patch
Details | Diff | Splinter Review
2.76 KB, patch
Details | Diff | Splinter Review
6.72 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
If a ton of JS gets attached to a live DOM, then all other references to it are dropped, the JS will get marked gray by the GC, causing the CC to trace through it all the time, even though everything reachable from it is live.  The CC has to do this because if there are two gray paths to a JS object, one from a live object and one from a dead object, we have to trace them both so we know the JS object is live, not dead.

Bill suggested a solution to this problem: we can mark JS objects that are held by obviously live C++ objects (like DOM nodes in currently active documents) black instead of gray.  Any such black marked objects aren't traced by the CC.  This is safe to do because the GC guarantees (in theory) that there are no black-gray edges.

Bill and I saw at least one instance of this in blassey's CC log (he's experiencing 1+ second CCs that are mostly JS).  I need to do some analysis to see how many nodes could be skipped through this method, and figure out what kind of JS-holders are the most common source of this problem.

This would take some pressure off the CC tracing JS infrastructure, hopefully reducing the need for having the CC use the JS engine's marker.

To start with, this could just check the existing document InGeneration stuff for obvious-liveness, but going forward it could hook into smaug's CanSkip infrastructure for a more general way for objects to report that they are definitely alive.

One tricky aspect of this is that we have to do all black marking before we do gray marking, which means that we'll have to iterate over all XPC roots twice.
(Assignee)

Comment 1

7 years ago
A drawback is that it will delay the freeing of objects in some circumstances: a JS object marked black in this manner won't be freeable even after the DOM is released until the next GC.  If the black JS holds onto the DOM in turn, then the DOM will stay around until the next GC.  Probably not a huge deal in practice.
(Assignee)

Comment 2

7 years ago
On further reflection, we shouldn't use CanSkip.  We have to limit ourselves to very strong reasons, like InGeneration.  CanSkip may do things like return true if any element of the current element's document is held onto by a black JS object.  Hmm, well, mark bits won't even be set when we're in the GC, so it wouldn't even make sense to use anything that will query them.  So, yes, let's just start with some kind of InGeneration-ish thing and go from there.
Yeah, CanSkip is too heavy weight. I'm just adding IsBlack() to nsXPCOMCycleCollectionParticipant
(for other reasons). That could be useful here too.
(Assignee)

Comment 4

7 years ago
I wrote a simple analyzer of C++ things that hold JS objects.  There are a couple of options to control the output, which you can see with -h.

https://github.com/amccreight/heapgraph/blob/master/cc/js_holders.py
(Assignee)

Comment 5

7 years ago
Here's are the top results from blassey's CC log (he was having very slow CC times due to a ton of JS):
   16975 nsGenericElement (xhtml)
   13273 XPCWrappedNative
    3726 nsGenericDOMDataNode
    3086 nsGenericElement (XUL)
    2897 nsXPCWrappedJS
    1748 nsJSEventListener
(rest were < 128)

This is just the number of objects of each type that held JS pointers.
(Assignee)

Comment 6

7 years ago
For CWW's log (those with > 375):
   18691 XPCWrappedNative
   15542 nsGenericElement (xhtml)
    8587 nsXPCWrappedJS
    3650 nsGenericDOMDataNode
    1286 nsGenericElement (XUL)
    1257 XPCVariant
(Assignee)

Comment 7

7 years ago
These are the largest groups from CWW's log when split up (I also removed the ones like nsGenericDOMDataNode that aren't any different from the above list):
    6352 nsXPCWrappedJS (nsIDOMEventListener)
    3650 XPCWrappedNative (Text)
    3600 nsGenericElement (xhtml) li
    3541 XPCWrappedNative (HTMLLIElement)
    3346 nsGenericElement (xhtml) a
    3271 XPCWrappedNative (HTMLAnchorElement)
    3181 nsGenericElement (xhtml) div
    2987 XPCWrappedNative (HTMLDivElement)
    2708 nsGenericElement (xhtml) span
    2695 XPCWrappedNative (HTMLSpanElement)
    1614 nsXPCWrappedJS (nsISupports)
     822 nsGenericElement (xhtml) form
     821 XPCWrappedNative (HTMLFormElement)
     784 nsGenericElement (xhtml) ul
     784 XPCWrappedNative (HTMLUListElement)
     606 nsGenericElement (xhtml) input
     604 XPCWrappedNative (HTMLInputElement)
(Assignee)

Comment 8

7 years ago
Same thing for blassey's log:
    5762 nsGenericElement (xhtml) span
    4067 XPCWrappedNative (HTMLSpanElement)
    3726 XPCWrappedNative (Text)
    2929 nsGenericElement (xhtml) div
    2478 nsGenericElement (xhtml) li
    2440 nsGenericElement (xhtml) a
    1835 XPCWrappedNative (HTMLAnchorElement)
    1651 XPCWrappedNative (HTMLDivElement)
    1454 nsXPCWrappedJS (nsIDOMEventListener)
    1113 nsGenericElement (xhtml) img
     763 nsGenericElement (XUL) image
     671 nsGenericElement (XUL) label
     587 nsGenericElement (xhtml) td
     518 nsXPCWrappedJS (nsIDOMXULLabelElement)
     516 XPCWrappedNative (HTMLImageElement)
Similar classes.  It looks like if we could blot out XPCWrappedNatives and a few element classes that would get rid of a lot of nodes.
(Assignee)

Comment 9

7 years ago
Smaug pointed out in IRC that we'd have to rev the CC generation in a GC if we want to use that information.  I guess this would be by making nsCCUncollectableMarker::Observe observe a GC, too?  Assuming that's a thing we can observe.
(In reply to Andrew McCreight [:mccr8] from comment #9)
> I guess this would be by making
> nsCCUncollectableMarker::Observe observe a GC, too?  Assuming that's a thing
> we can observe.
Someone would need to send the notification, but yes, that sounds doable.
We could add garbage-collector-begin, or perhaps cycle-collector-end notification
depending on when we should mark documents to be in generation.
(Assignee)

Comment 11

7 years ago
Created attachment 588173 [details] [diff] [review]
Mark black from InGeneration JS holder elements during GC marking. WIP.

Here's what I was thinking this might look like.  It adds an extra pass over the JS holders, during black XPC root marking.  If the JS holder is an element, we check if it is in a currently-live document.

Elements are probably the single biggest source of a win from this approach.  It is unlikely that the JS Objects associated with XPCWrappedNatives hold onto much JS.  But I should confirm that.  wrapped JS would probably include a fair amount, but there's less of that, and it is probably harder to figure out if it is being held by a certainly-live object.

One annoying detail is that InGeneration wants a cycle collector callback just to look up WantAllTraces, but we want to always perform this optimization (the CC will later ignore that objects are black, so this optimization should not  perturb the result of WantAllTraces CC).  Right now I passed in a null reference, but I should fix it to take a pointer or something.

This patch also does not rev the generation number.  I think that will only make things survive for an extra CC, so it may not be the end of the world for the purposes of experimentation.
Comment on attachment 588173 [details] [diff] [review]
Mark black from InGeneration JS holder elements during GC marking. WIP.

>   static bool InGeneration(nsCycleCollectionTraversalCallback &cb,
>                              PRUint32 aGeneration) {
>-    return !cb.WantAllTraces() &&
>+    // change this to a Callback* instead of &!
>+    return (!&cb || !cb.WantAllTraces()) &&
>            aGeneration &&
>            aGeneration == sGeneration;
>   }
In some of my patches there is a version without cb parameter


>+static bool HolderMustBeAlive(nsISupports *holder)
>+{
>+    nsresult rv;
>+    nsCOMPtr<nsINode> node = do_QueryInterface(holder, &rv);
This causes each node here ending up to purple buffer.
We should try to avoid that

This code could QI to nsCycleCollectionISupports and then nsXPCOMCycleCollectionParticipant
and we could add something like IsBlackGCRoot() to that class.
Created attachment 588179 [details]
test case

Here's a really simple test case I wrote. Currently I get CCs that are ~6 seconds. If we can get this stuff marked black instead of gray, I think the CC time should be tiny.
(Assignee)

Comment 14

7 years ago
Bill tried something like my patch, and got crashes, maybe because some things we trace aren't actually nsISupports.  I think there are a few native classes that aren't but still participate in cycle collection.
(Assignee)

Comment 15

7 years ago
> This code could QI to nsCycleCollectionISupports and then
> nsXPCOMCycleCollectionParticipant
> and we could add something like IsBlackGCRoot() to that class.

Or CanSkipInGC(), to match with the other stuff you are adding.
(Assignee)

Comment 16

7 years ago
Hmm, okay that's a bad name, never mind.  But yeah that may be the way to go.
(Assignee)

Comment 17

7 years ago
Created attachment 588219 [details] [diff] [review]
add LiveInGC hook to nsScriptObjectTracer.  WIP.

This follows the strategy of Smaug's CC patches, and adds a new callback LiveInGC to nsScriptObjectTracer.  If this returns true, then we run a trace on the holder during black marking.  Probably doesn't quite compile, as I think I'm missing a definition of LiveInGC in at least one case.  I haven't defined the function anywhere yet, but it should be straightforward.
Attachment #588173 - Attachment is obsolete: true
(Assignee)

Comment 18

7 years ago
Created attachment 588231 [details] [diff] [review]
fix some CC macro gunk, implement nsINode::LiveInGC
Attachment #588219 - Attachment is obsolete: true
(Assignee)

Comment 19

7 years ago
Created attachment 588316 [details] [diff] [review]
fixed CC macro gunk, now it actually builds and runs, WIP.

I messed around with the CC macro gunk some more so now it actually builds and runs.  I'd added support for this to nsGenericElement, but after I did a test run, it looked like nsDocument was also a prime possible source of improvement, so I added that, too.
try run without nsDocument: https://tbpl.mozilla.org/?tree=Try&rev=c807d9a9ba97
try run with nsDocument: https://tbpl.mozilla.org/?tree=Try&rev=7d9a15b32019

The version with nsDocument seemed to have a lot less JS stuff in the graph than the version without, but I haven't done very scientific testing.  There were still a few thousand JS objects in the graph, so this isn't a silver bullet, but maybe it is an improvement at least.
Attachment #588231 - Attachment is obsolete: true
(In reply to Bill McCloskey (:billm) from comment #13)
> Created attachment 588179 [details]
> test case
> 
> Here's a really simple test case I wrote. Currently I get CCs that are ~6
> seconds. If we can get this stuff marked black instead of gray, I think the
> CC time should be tiny.
Using my latest tryserver builds (and having 141 tabs open), I get 45ms CC time.
(Assignee)

Comment 21

7 years ago
smaug: Do your CanSkipSelf methods check GC mark bits?  I guess they probably do.

For this bug, I need to check out Bill's test case, and do some basic measurement of how much JS this patch actually avoids.  One way to do that would be to analyze a CC graph without this optimization, to see how much it should clear.  Of course, as my JSContext exploration found, I really need to make sure that theory matches up with reality.  I guess I could write a custom MarkChildren thing.
(Assignee)

Comment 22

7 years ago
What this patch should do so far is mark black any JS that is reachable from a preserved wrapper.  A more natural candidate for it would be nsXPCWrappedJS, but I don't think the wrapped JS actually has any information about what C++ is keeping it alive, so I don't think we can do anything about it.  WrappedNatives looked like there might be some hope for skipping, but I haven't examined them too closely.
(In reply to Andrew McCreight [:mccr8] from comment #21)
> smaug: Do your CanSkipSelf methods check GC mark bits?  I guess they
> probably do.
CanSkipThis does check GC mark bits in DOM if the bits are available.
Same could be added for XHR etc.
(Assignee)

Comment 24

7 years ago
I did some rough analysis on a log file I had sitting around.  The script is called "live_js_analysis" on my github repo.  I considered a C++ object to be "live" if it had a single outgoing edge called "Preserved wrapper".  These were all nsDocuments and nsGenericElements.  Basically, this will only happen for these two classes if the CC has decided the nodes are live.  I then computed the number of JS objects reachable from those JS objects without crossing into C++ objects.

There were 1218 JS objects held in preserved wrappers, and 79061 JS objects held directly or indirectly by those objects.  That left only 5183 objects.  Interestingly, nsDocuments and nsGenericElements hold onto a slightly different set of objects.  Just nsGenericElements gives you 61668 objects pruned from the graph (22576 remaining), and just nsDocuments gives you 17393 objects pruned from the graph (66851 remaining).

I then used my live_census script to analyze the contents of two CC logs I generated with my pruning scripts applied.  This only shows the number of objects that we failed to remove.  With just nsGenericElement support, there were about 25000 JS objects in the graph.  With nsDocument support added, the number dropped to around 3500.  These numbers of residual objects match up closely to the residual counts predicted by my analysis of a CC graph without this optimization.

This suggests that at least for a simple browsing session the optimization in this patch will eliminate a lot of JS from the graph.
Comment on attachment 588316 [details] [diff] [review]
fixed CC macro gunk, now it actually builds and runs, WIP.


>   static bool InGeneration(nsCycleCollectionTraversalCallback &cb,
>                              PRUint32 aGeneration) {
>-    return !cb.WantAllTraces() &&
>+    // change this to a Callback* instead of &!
>+    return (!&cb || !cb.WantAllTraces()) &&
>            aGeneration &&
>            aGeneration == sGeneration;
>   }
Better to just have InGeneration which doesn't take cb

> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsGenericElement)
>   nsINode::Trace(tmp, aCallback, aClosure);
> NS_IMPL_CYCLE_COLLECTION_TRACE_END
> 
>+
>+
Extra new lines

>+// maybe SkipInGC is an okay name.  It is kind of weird because we
>+// don't skip in the GC if this is true, but it means that when we are
>+// in the GC, we can see if we will be able to skip it later in the
>+// CC.
I still wonder if it was safer to run forgetSkippable right before using LiveInGC.
That way documents would have valid generation.


>+static JSDHashOperator
>+TraceLiveJSHolder(JSDHashTable *table, JSDHashEntryHdr *hdr, uint32_t number,
>+              void *arg)
>+{
>+    ObjectHolder* entry = reinterpret_cast<ObjectHolder*>(hdr);
why reinterpret_cast? Would static_cast be just enough
(Assignee)

Comment 26

7 years ago
(In reply to Olli Pettay [:smaug] from comment #25)
> Better to just have InGeneration which doesn't take cb
That's true.

> >+// maybe SkipInGC is an okay name.  It is kind of weird because we
> >+// don't skip in the GC if this is true, but it means that when we are
> >+// in the GC, we can see if we will be able to skip it later in the
> >+// CC.
> I still wonder if it was safer to run forgetSkippable right before using
> LiveInGC.
> That way documents would have valid generation.

I still need to add code to do the generation stuff.  I was thinking maybe adding a new event at the start of a GC and then getting the generation setter to listen to it.

I also need to figure out how to integrate the new callback and related macros with bug 716518.  It is awkward because mJSHolders apparently holds nsScriptObjectTracer not nsXPCOMCycleCollectionParticipant.  Also whether I should bother with the bool flag optimization to avoid virtual calls.

> why reinterpret_cast? Would static_cast be just enough
Probably.  I'll try it.
(Assignee)

Comment 27

7 years ago
It looks like all existing JS holders are actually nsXPCOMCycleCollectionPartipants...
(Assignee)

Comment 28

7 years ago
Oops, no, there's exactly one other kind of JS holders: nsXULPrototypeNode and descendents.  Oh well, I'll probably end up moving the boolean flag from bug 716518 stuff up to script holder.

I ran my analysis script on blassey's slow CC log.  That calculates that there are about 200,000 JS objects that would be avoided with this optimization.  Of course, that leaves another 540,000, so this isn't a silver bullet.

For Cww's slow CC log, this optimization would only skip about 18,000 nodes, leaving another 700,000 JS nodes in the graph, which is almost nothing.  I guess his problem is slightly different.

This optimization doesn't help if live DOM holds a tiny amount of JS, which holds a disconnected DOM, which then holds a ton of JS.
(In reply to Andrew McCreight [:mccr8] from comment #28)
> This optimization doesn't help if live DOM holds a tiny amount of JS, which
> holds a disconnected DOM, which then holds a ton of JS.

Is that the issue that is responsible for the remaining nodes in Cww's and blassey's dumps?

Also, I don't understand why the optimization won't help in this case. Wouldn't we just mark all that stuff black?
(In reply to Andrew McCreight [:mccr8] from comment #28) 
> This optimization doesn't help if live DOM holds a tiny amount of JS, which
> holds a disconnected DOM, which then holds a ton of JS.
This is where CanSkip should be able to help, since it detects that disconnected DOM black and marks all the
JS edges from it black.
(Assignee)

Comment 31

7 years ago
(In reply to Bill McCloskey (:billm) from comment #29)
> Is that the issue that is responsible for the remaining nodes in Cww's and
> blassey's dumps?

I don't know.  I should try to analyze that.

> Also, I don't understand why the optimization won't help in this case.
> Wouldn't we just mark all that stuff black?

The way the current optimization works is that a node checks if it is held by a document in a live window.  (A node in a disconnected DOM isn't in any document, so the check won't work.) That node floods black.  If during flooding we reach another DOM, we don't trace through it to find further JS being held onto by that DOM to flood black.  That seems a bit heavyweight to me, though I think Smaug's patches do something similar outside of the GC.  Doing it in the GC is nice because it is going to be much faster.

Although, I could imagine maybe adding something to the trace hook of a wrapped native for DOM stuff that designates the entire DOM as being marked, mapping the topmost elements of DOMs to the set of elements they have that are wrappers, and then doing some kind of iterative process.  Still seems pretty intense.
(Assignee)

Comment 32

7 years ago
(In reply to Olli Pettay [:smaug] from comment #30)
> This is where CanSkip should be able to help, since it detects that
> disconnected DOM black and marks all the
> JS edges from it black.

Right, your stuff helps cover these cases, but it is more expensive, as things will have to be marked gray by the GC first, then later marked black by the slower unmarkGray path.  For the optimization in this patch, the only added cost is an additional traversal over JS holders, which is 1/80th of the number of nodes in my one test.
(Assignee)

Comment 33

7 years ago
nsGenericDOMDataNode are another source of preserved wrappers that I failed to notice because my scripts were buggy somehow.  They don't substantially change the number of nodes eliminated for the two slow CC logs.  That only prunes another few thousand nodes.
(Assignee)

Comment 34

7 years ago
Updated my patch a bit, and finally tried it out on Bill's example.  On the upside, CCs remain fast (under 100ms).  On the downside, GCs start taking two seconds.

On my normal nightly with Bill's example, CCs take 11 seconds, but the GC is only taking 500ms.  I should figure out why this slows down the GC so much, and if that can be avoided somehow.
(Assignee)

Comment 35

7 years ago
Created attachment 590433 [details] [diff] [review]
part 1: add CanSkipInGC

Like the other CanSkip function, except it will be called in the GC, and thus can't examine mark bits.
Attachment #588316 - Attachment is obsolete: true
(Assignee)

Comment 36

7 years ago
Created attachment 590434 [details] [diff] [review]
Use CanSkipInGC hook to mark some JS black not gray, WIP
(Assignee)

Comment 37

7 years ago
Created attachment 590436 [details] [diff] [review]
part 3: make documents and elements skippable-in-gc
(Assignee)

Comment 38

7 years ago
Created attachment 590438 [details] [diff] [review]
Add observer for GC tracing, WIP

This adds a new observer for XPC black root tracing, so the document generations will be up to date.
(Assignee)

Comment 39

7 years ago
(In reply to Andrew McCreight [:mccr8] from comment #34)
> On my normal nightly with Bill's example, CCs take 11 seconds, but the GC is
> only taking 500ms.  I should figure out why this slows down the GC so much,
> and if that can be avoided somehow.
Turned out this was just because I was comparing a debug build to a non-debug build.  When I changed my local build to not do anything new in the GC, the times were the same.

I also measured how long it took to do the black marking phase, and it was something like .3ms, compared to about .6ms.  It also appeared that the cost was proportional to the number of things it found, so the main overhead appears to be calling the Trace hook, which is good.
(Assignee)

Comment 40

7 years ago
Created attachment 590461 [details] [diff] [review]
Use CanSkipInGC hook to mark some JS black not gray, WIP
Attachment #590434 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #590438 - Attachment is obsolete: true
I think this could actually help a lot with my current cc logs.

Are all those 3 patches needed? I guess so.
I'll update the patches to work with my patches and upload to try.
(Assignee)

Comment 43

7 years ago
Yes, you need all three.

Don't bother trying yet, there's still something wrong with the patch.  I did a try run: https://tbpl.mozilla.org/?tree=Try&rev=366633ffd3c9

The Mochitests are fine, but it is crashing repeatedly during XPCShellTest.  The opt stacks look like something is accessing freed memory, but the debug stacks look like maybe the nsCCUncollectableMarker::Observe during the GC is trying to call into JS or something?  I don't know what is going on there, or why it would happen during the XPCshell tests but not otherwise.

 1  libxul.so!CrashInJS [jsutil.cpp : 98 + 0xb]
 2  libxul.so!JSAutoEnterCompartment::enter [jsapi.cpp : 243 + 0x2e]
 3  libxul.so!nsXPCWrappedJSClass::CallMethod [XPCWrappedJSClass.cpp : 1204 + 0xb]
 4  libxul.so!nsXPCWrappedJS::CallMethod [XPCWrappedJS.cpp : 611 + 0x12]
 5  libxul.so!PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp : 92 + 0x12]
 6  libxul.so!nsCCUncollectableMarker::Observe [nsCCUncollectableMarker.cpp : 239 + 0xb]
Too late...

Looks like the patch helps with the cleanup stuff happening right after GC.
But is it GC times is a bit higher.

We could actually do that black marking asynchronously...which is close to what CanSkip ends up
doing, though it may not black-mark as much as this approach.
(Assignee)

Comment 45

7 years ago
I think I figured out why XPCShell tests were failing.  It turns out that a number of XPCShell tests implement a fake window watcher in JS.  nsCCUncollectableMarker::Observe accesses the window watcher.  My patch adds a call to Observe inside of the GC.  We're not allowed to access JS during a GC.  Thus, things fall apart.

I looked at four of the tests that are failing, and they all do this.  Here's an example: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bug594058.js

I'm not really sure how to work around this.  I'd guess there's no way to see if the window watcher is JS or not.  Lifting the Observe out of the GC itself would require adding another hook, as the JS engine itself can decide to run a GC, and that is pretty gross.

Maybe it isn't so bad to just not have the hook.  It shouldn't affect shutdown because the XPCOM shutdown observe will mark all windows as visible.  The only thing it will do is that if there's a cycle between a DOM in a live window and some JS, then when the window gets closed then it will take an additional CC before a GC before it will get taken apart.  Maybe we could trigger an observe when the set of live windows and whatever else decreases?
(In reply to Andrew McCreight [:mccr8] from comment #45)
> I think I figured out why XPCShell tests were failing.  It turns out that a
> number of XPCShell tests implement a fake window watcher in JS.
Huh, sounds like an insane test.
XPCShell tests have also few other test which implement interfaces which really
should be C++ only in JS. (IIRC some test implements nsIDOMWindow in JS.)
 

> I'm not really sure how to work around this.
Disable the test and if it is testing something important, convert to chrome test?
(Assignee)

Comment 47

7 years ago
(In reply to Olli Pettay [:smaug] from comment #46)
> > I'm not really sure how to work around this.
> Disable the test and if it is testing something important, convert to chrome
> test?

Well, as long as that interface is scriptable, then potentially an addon could hook itself in and apparently cause crashes in js::gc::Arena::finalize<JSObject>, which is what was happening in opt builds.

Instead of iterating over the windows when we CC, could we register the nsCCUncollectableMarker with the window watcher, and then update things as necessary as windows open and close?  In fact, instead of using generations, you could just add a flag that gets set true or false.  On XPCOM shutdown, then you'd iterate over all windows and set them to false, or just set a SHUTDOWN flag to false that always returns false.  Though that sounds like a fairly huge change to make just to make open/close window information slightly more precise.
(Assignee)

Comment 48

7 years ago
Created attachment 590579 [details] [diff] [review]
part 2: mark JS held by skippable things black
Assignee: general → continuation
Attachment #590461 - Attachment is obsolete: true
(In reply to Andrew McCreight [:mccr8] from comment #47)
> 
> Well, as long as that interface is scriptable,
Do you mean non-builtinclass ?
(Assignee)

Comment 50

7 years ago
(In reply to Olli Pettay [:smaug] from comment #49)
> Do you mean non-builtinclass ?

Ah, yes.  I wonder if there's a way to somehow check that the GC isn't running any time we execute a non-builtinclass, even if it is implemented in C++.
(Assignee)

Comment 51

7 years ago
Created attachment 590583 [details] [diff] [review]
part 4: make nsXBLDocumentInfo skippable-in-gc

nsXBLDocumentInfo hold onto a fair amount of JS crud in my tests.

Remaining JS holders in my log:
     187 nsJSEventListener
     148 nsXPCWrappedJS
      31 JSContext
      27 XPCWrappedNative
      24 nsXULPrototypeNode
      10 nsJSScriptTimeoutHandler
       2 nsGenericElement (xhtml) iframe http://www.cnn.com/
       2 nsGenericElement (XUL) box chrome://global/content/console.xul

Not obvious to me how to tell if any of these are being held by live DOM or whatnot.

I also confirmed that dropping the Observe from the GC is not awesome.  When you load up Bill's test example, you need a full sequence of CC->GC to get fast CCs.  With the sequence load page->GC->CC->CC->CC (manually invoked the latter two CCs), all of the CCs are 13 seconds.
(Assignee)

Comment 52

7 years ago
There were about 7600 JS things in the graph.

XPCWrappedNatives held alive 6341
JSContext held alive 6392
nsJSEventListener held alive 3782
nsXPCWrappedJS held alive 806
nsXULPrototypeNode held alive 108
nsJSScriptTimeoutHandler held alive 24

The first free apparently keep alive a lot of it, but it isn't as obvious how to decide it is okay to skip them in the CC.

(according to my analysis of a previous graph, adding nsXBLDocumentInfo makes about 500 more out of 6000 JS things to be marked black)
(Assignee)

Comment 53

7 years ago
The two xhtml elements that were still JS holders only held onto a total of 2 JS objects.  It looks like just their slim wrappers weren't marked.

The two XUL elements that were still JS holders were garbage, which explains why they are traced through.
(Assignee)

Updated

7 years ago
Attachment #590433 - Attachment description: Add CanSkipInGC hook, WIP → part 1: add CanSkipInGC
(Assignee)

Updated

7 years ago
Attachment #590436 - Attachment description: Use the CanSkipInGC hooks for documents and generic elements → part 3: make documents and elements skippable-in-gc
(Assignee)

Comment 54

7 years ago
Bill said I could put the nsCCUncollectable stuff in XPCJSRuntime::GCCallback under JSGC_BEGIN, and that is run outside of the GC per se, so we should be able to safely call JS in there.
(Assignee)

Comment 55

7 years ago
Created attachment 590955 [details] [diff] [review]
part 1: add CanSkipInGC

The (void)tmp things are to prevent unused variable warnings for CanSkip methods with empty bodies.
Attachment #590433 - Attachment is obsolete: true
(Assignee)

Comment 56

7 years ago
Created attachment 590960 [details] [diff] [review]
part 2: make GC use CanSkipInGC, add gc begin NotifyObservers
Attachment #590579 - Attachment is obsolete: true
(Assignee)

Comment 57

7 years ago
Created attachment 590966 [details] [diff] [review]
part 3: make nsUncollMarker listen to GC, add new InGeneration
(Assignee)

Comment 58

7 years ago
Created attachment 590970 [details] [diff] [review]
part 4: make docs, elements and XBLDocInfo skippable-in-gc
Attachment #590436 - Attachment is obsolete: true
Attachment #590583 - Attachment is obsolete: true

Comment 59

7 years ago
Try run for 08d8dd5a6596 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=08d8dd5a6596
Results (out of 184 total builds):
    success: 157
    warnings: 25
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amccreight@mozilla.com-08d8dd5a6596
(Assignee)

Comment 60

7 years ago
I think this has come together, but with all the craziness with Olli's CC patches and incremental GC coming down the pipe in the next few week, I think I'll hold off on it for a bit.
(Assignee)

Updated

5 years ago
Blocks: 827392
(Assignee)

Updated

10 months ago
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.