Closed Bug 717500 Opened 12 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 11 obsolete files)

test case
299 bytes, text/html
Details
part 1: add CanSkipInGC
7.59 KB, patch
Details | Diff | Splinter Review
part 2: make GC use CanSkipInGC, add gc begin NotifyObservers
5.46 KB, patch
Details | Diff | Splinter Review
part 3: make nsUncollMarker listen to GC, add new InGeneration
2.76 KB, patch
Details | Diff | Splinter Review
part 4: make docs, elements and XBLDocInfo skippable-in-gc
6.72 KB, patch
Details | Diff | Splinter Review 
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.
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.
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.
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
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.
For CWW's log (those with > 375):
   18691 XPCWrappedNative
   15542 nsGenericElement (xhtml)
    8587 nsXPCWrappedJS
    3650 nsGenericDOMDataNode
    1286 nsGenericElement (XUL)
    1257 XPCVariant
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)
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.
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.
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.
Attached file 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.
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.
> 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.
Hmm, okay that's a bad name, never mind.  But yeah that may be the way to go.
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

        Attachment #588219 -
        Attachment is obsolete: true

    
    

    
  


  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
(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.

    
    

    
  
It looks like all existing JS holders are actually nsXPCOMCycleCollectionPartipants...

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 1: add CanSkipInGC (obsolete)
        — 
          — Splinter Review
      

    


  
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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add observer for GC tracing, WIP (obsolete)
        — 
          — Splinter Review
      

    


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

    
    

    
  
(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.

    
    

    
  


  

        Attachment #590434 -
        Attachment is obsolete: true

    
  

        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.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
(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: 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 ?

    
    

    
  
(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++.

    
    

    
  


  
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.

    
    

    
  
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)

    
    

    
  
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.

    
  

        Attachment #590433 -
        Attachment description: Add CanSkipInGC hook, WIP → part 1: add CanSkipInGC

    
  

        Attachment #590436 -
        Attachment description: Use the CanSkipInGC hooks for documents and generic elements → part 3: make documents and elements skippable-in-gc

    
    

    
  
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.

    
    

    
  


  
The (void)tmp things are to prevent unused variable warnings for CanSkip methods with empty bodies.

        Attachment #590433 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #590436 -
        Attachment is obsolete: true

        Attachment #590583 -
        Attachment is obsolete: true

    
    

    
  
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

    
    

    
  
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.

    
  
Blocks: 827392

    
  
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: