Closed
Bug 549806
Opened 15 years ago
Closed 14 years ago
disentangle cycle collector and JS GC
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
People
(Reporter: gal, Assigned: gal)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 21 obsolete files)
|
34.00 KB,
patch
|
Details | Diff | Splinter Review |
CC and GC engage in a intricate dance with subtle JSGCCallback dependencies in order to avoid establishing a refcnt for JSObjects. Instead, we unroot globals and see what is alive without them. (Rest of the description copied from bug 549793, which was the result of a discussion with dbaron). This bug touches all the relevant parts of the context global clearing. https://bugzilla.mozilla.org/show_bug.cgi?id=458099 I think one of the mistakes is that we clear the globals prior to the JS GC run. I assume this is an optimization attempt in order to avoid rescanning the JS GC heap again, however, we have to do that anyway to collect the cycle collector edges. Another potential benefit is that we immediately dispose of JS objects participating in a cycle the cycle collector drops. However, we pay a couple hefty fines for this: 1. CC becomes non-interruptible and non-abortable. We have to sweep the XPCOM heap to discover JSObjects that are held live through XPCOM and then mark them since we bypassed this marking by clearing out the context globals. 2. (1) muddles the boundary between GC and CC and results in a "GC marking done, but not really" state where the JS GC thinks its done marking, but the CC will revive additional objects. I would like to undo this optimization attempt and run a regular JS GC and then CC only on the XPCOM side. We still have to explore the JS heap to a degree to identify cycles, but the only cycle resolution we engage in is removing roots of cycles at the XPCOM->JSHeap boundary. Once that root is dropped the next JS GC will clear the cycle on the JS side, unwinding the cycle on the XPCOM side. This has a number of benefits: 1. Simple, clearly defined GC protocol. Marking, Sweep, Done. No intermediate and mark after IsAboutToBeFinalized calls. 2. CC becomes interruptible and abortable, i.e. if the user moves the mouse or presses a key or we get a network update, we just stop CCing and do it later. With this we can CC more often, but essentially with 0 latency (minus battery life issues, but that's for stuart to worry about). 3. The JS engine can start sweeping its own heap once its done marking, concurrently to cycle collection since cycle collection will no longer revive dead objects (or try to touch dead objects). We spend more time sweeping than marking, so we might be able to hide the time of CC in the sweep time. A bit of archeology shows that way back we decided to intertwine GC and CC to avoid a redundant scan/mark. That's basically exactly what I propose we try now, since we can make it concurrent and interruptible. We will need some experimenting with this to see whether comment #1 is faster, or the decision back in 2007 was right after all. Doesn't look super hard to hack that up though (at least to he point where we can benchmark it), since its mostly undoing changes. "So if I understand correctly XPCOM objects would use the API from bug 379220 to keep JS objects alive. Instead of running the JS GC and use gcThingCallback, we'd call JS_TraceRuntime to get refcounts for JS objects and we'd use tracing to reconstruct the graph (like in attachment 261948 [details] [diff] [review] [details]). If that's correct, then we would duplicate a bunch of work between the JS GC (which we'll still need to run) and the refcounting trace?" https://bugzilla.mozilla.org/show_bug.cgi?id=377884#c9
|
Assignee | |
Comment 1•15 years ago
|
||
To decide whether this makes sense at all we need to know how expensive it is to determine the refcnt for all JSObjects. That sparked the avalanche of patches that resulted in the current quantum entanglement. I will hack up a patch and measure that.
|
||
Comment 2•15 years ago
|
||
(In reply to comment #0) > I think one of the mistakes is that we clear the globals prior to the JS GC > run. I assume this is an optimization attempt in order to avoid rescanning the > JS GC heap again, however, we have to do that anyway to collect the cycle > collector edges. This is not an optimization. The globals stored in JSContext::globalObject are not roots. For example, if there is no JS code running on any thread, there should be no JS roots. Yet JS_GC() will mark the globals for API compatibility. Hence that hack to skip then aside during the CC and only GC-mark them if CC finds them alive.
|
|
||
Comment 3•15 years ago
|
||
(In reply to comment #0) > I would like to undo this optimization attempt and run a regular JS GC and then > CC only on the XPCOM side. Again, if CC runs only when there is no thread that executes the JS, then the would be no need to do any integration work with JS GC besides allowing to navigate the GC heap so the CC can detect edges between XPCOM objects that goes into JS heap. The trouble starts when the is JS code somewhere. Then the JS roots on the stack should prevent any cycle that goes via them to be collected. Still we can avoid the JS_GC() if the cycle collector would simply learn about the current JS roots and treat them specially. But this roots can be trivially discovered via calls js_TraceRuntime().
|
|
Assignee | |
Comment 5•15 years ago
|
||
The attached patch determines the precise refcnt for every JSObject that we come across. refcnt 0 and 1 are held in the bits only, > 1 is held in an 4mb array that is used as a hash table with accepted collisions. Collisions cause the refcnt to be overestimated, which causes the JSObject to be considered always live by the cycle collector (since mInternalRefcnt < mRefcnt). Collisions should be relatively rare. The 4mb array is large enough to represent all objects in our default 32mb heap with few collisions (32mb heap are sufficient for 1mil objects). The overhead for getting the precise refcnts is 9% for the test case in the bug over a GC without refcnt calculation, plus we need an extra 4mb memory (temporarily). The CC itself also uses a ton of memory, so the latter might not be a big deal. Remains to be seen. Based on sharking gmail and chrome experiments, T_CC + T_GC == 2 * T_GC, so if we can get the refcnts in 1.1 * T_GC and then run the cycle collector in parallel to the GC sweeping, we might come out ahead. Waiting for Igor to elaborate on the API compatibility issue. API compatibility. Never far away when something is slow.
Assignee: nobody → gal
|
|
||
Comment 6•15 years ago
|
||
(In reply to comment #4) > Could you elaborate on the compatibility requirement there? THe JS engine promises that JSContext::globalObject is a root. But it should not be one for the CC purposes if JSContext::globalObject is referenced from a possible dead cycle participant.
|
|
||
Comment 7•15 years ago
|
||
(In reply to comment #5) > Created an attachment (id=429984) [details] > patch > > The attached patch determines the precise refcnt for every JSObject that we > come across. The CC does not need to know the reference count for JS objects! It needs to know only the number of edges that goes into a possible cycle participant from JS objects that are reachable from other cycle participants and not reachable from JS roots. As such there is no need to do any reference counting of JS objects, it is sufficient just to learn the JS objects that are reachable from the cycle participants and then enumerate them to account the edges into the XPCOM world. Such data structure can be efficiently implemented using one flag bit per object and a word per JSGCArena to link all GC arenas that contains objects referenced from the XPCOM world. > Based on sharking gmail and chrome experiments, T_CC + T_GC == 2 * T_GC, so if > we can get the refcnts in 1.1 * T_GC and then run the cycle collector in > parallel to the GC sweeping, we might come out ahead. If the CC runs with some thread running JS script, it must learn about any potential cycle participant XPOM object that is reachable from JS roots. Without using JS_GC the code would just duplicates the efforts. Now the price for that is an extra marking that has to be done after the cycle collector has detected live XPCOM objects and the following JS finalization that increases the CC pause. If this extra marking + finalization contributes significantly to the CC time, then we can consider avoiding it like abandoning that stage. So could you check the timing split between the initial JS_GC invocation, the cycle collection and the final GC marking and finalization?
|
|
Assignee | |
Comment 8•15 years ago
|
||
What do you mean with extra marking? The terms are flying around a bit here. Lets try to define what we mean: mark = mark phase of the JS GC sweep = sweep phase of the JS GC walk = walking the JS heap graph using the JSTracer API Lets avoid the term "participant" since its used to name something else in the implementation. Could you rephrase or clarify this sentence (if you need more terms please define them): "Without using JS_GC the code would just duplicates the efforts. Now the price for that is an extra marking that has to be done after the cycle collector has detected live XPCOM objects and the following JS finalization that increases the CC pause." The proposed process is to do a JS_GC mark phase, collect all the information the CC needs, and then proceed to sweep in parallel to CC. At this point marking is over. No more marking. By nobody. Not the JS engine. Not the CC. Sweeping has started. To late to mark. Everything is already marked. All the CC can do at this point is break/unlink roots, which then will unravel during the next JS_GC run. Cycles involving JS Objects are not immediately collected. Since cycles are long-lived, this mild delay until the cycle gets collected should be immaterial. I will do some measurements vs distribution between mark and sweep. Last time I checked, sweep time still dominates.
|
||
Comment 9•15 years ago
|
||
(In reply to comment #5) > Waiting for Igor to elaborate on the API compatibility issue. API > compatibility. API, SchmAPI. See JSOPTION_UNROOTED_GLOBAL, which is part of the API. It's used: https://mxr.mozilla.org/mozilla-central/search?string=JSOPTION_UNROOTED_GLOBAL&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central > Never far away when something is slow. True often enough, still a cheap shot and logically untenable. When tracing misfires due to a bug in jstracer.cpp, something can be very slow, but API compatibility is far away. Please think about API like you would any working relationship with a large number of people. Not a promise forever but a contingent set of arrangements that should not change all the time, but that can be improved by adding better APIs, then removing worse ones -- remember the branch callback. I see Igor's point: it is not necessary to compute all JS objects' refcounts. That is a sure loser if cycles involve relatively few JS objects. /be
|
|
||
Comment 10•15 years ago
|
||
(In reply to comment #8) > What do you mean with extra marking? The terms are flying around a bit here. > Lets try to define what we mean: > > mark = mark phase of the JS GC > sweep = sweep phase of the JS GC > walk = walking the JS heap graph using the JSTracer API > > Lets avoid the term "participant" since its used to name something else in the > implementation. Could you rephrase or clarify this sentence (if you need more > terms please define them): > > "Without using JS_GC the code would just duplicates the efforts. Now the price > for that is an extra marking that has to be done after the cycle collector has > detected live XPCOM objects and the following JS finalization that increases > the CC pause." The CC does not need to consider all XPCOM objects. It can consider only objects for which the reference count decreased from 2 or more. Only those object can form a dead cycle. These are the objects that I called "possible cycle participant". But the right term here is "gray objects". Currently the CC collection involves (not necessary in that order): 1. Initial GC marking to discover live JS objects that may reference XPCOM things. 2. CC walking from gray objects to discover all inner edges. 3. Color as unreachable the gray objects that are not reachable from live JS and XPCOM objects. 4. GC marking to mark JS things that are reachable from gray objects found live. 5. GC sweep In my previous comment I wanted to know how much time 4 and 5 takes since I though that phase 1 is inevitable. But now i realized that the phase 1 currently also marks strings and doubles. If we replace that by just marking the objects (that is, the only things that can refer into XPCOM), that may cut down the time of the phase 1 significantly. But that suggests to replace 1 with special-purpose flagging of objects (we borrow one of the currently wasted bits in JSGCArena) and then eliminate 4 and 5.
|
|
Assignee | |
Comment 11•15 years ago
|
||
> I see Igor's point: it is not necessary to compute all JS objects' refcounts.
> That is a sure loser if cycles involve relatively few JS objects.
Absolutely no intentions on actually doing that in production, but it was used to justify a design decision and I wanted to put a price tag on it.|
|
Assignee | |
Comment 12•15 years ago
|
||
> In my previous comment I wanted to know how much time 4 and 5 takes since I
> though that phase 1 is inevitable. But now i realized that the phase 1
> currently also marks strings and doubles. If we replace that by just marking
> the objects (that is, the only things that can refer into XPCOM), that may cut
> down the time of the phase 1 significantly. But that suggests to replace 1 with
> special-purpose flagging of objects (we borrow one of the currently wasted bits
> in JSGCArena) and then eliminate 4 and 5.
Yeah, that's exactly what I was thinking about. markBitmap has 3 flags left per JSObject. We could let a customer marker twiddle those. I think I would prefer sticking the entire mechanics into js_GC() with a simple API (MarkGray, IsGray or something like that, instead of calling back into the CC for every object we walk by and calling back into JS to set the mark bit).|
|
Assignee | |
Comment 13•15 years ago
|
||
Ok, Igor and I discussed this a bit further in irc. The additional marking can occur during the regular GC run, using the extra markBitmap bit. We start setting the other color in addition to the regular mark bit when we trace the rt->gcExtraRootsTraceOp part of the JS heap. The CC can start with the same roots and then follow this extra color to discover the relevant parts of the graph.
|
|
Assignee | |
Comment 14•15 years ago
|
||
Not meant to be reviewed, just a sketch.
Attachment #429984 -
Attachment is obsolete: true
|
|
||
Comment 15•15 years ago
|
||
(In reply to comment #12) > Yeah, that's exactly what I was thinking about. markBitmap has 3 flags left per > JSObject. We could let a customer marker twiddle those. I think I would prefer > sticking the entire mechanics into js_GC() with a simple API (MarkGray, IsGray > or something like that, instead of calling back into the CC for every object we > walk by and calling back into JS to set the mark bit). Is the idea to replace the following code in js_CallGCMarker : if (!IS_GC_MARKING_TRACER(trc)) { trc->callback(trc, thing, kind); goto out; } with something like: if (!IS_GC_MARKING_TRACER(trc)) { if (CC_MARKING_TRACER(trc)) { if (kind != JSTRACE_OBJECT && kind != JSTRACE_XML) goto out; MarkAsCCLive(); } else if (CC_WALKING_TRACER(trc)) { if (kind != JSTRACE_OBJECT && kind != JSTRACE_XML) goto out; if (IsCCLive()) goto out; MarkAsCCGrayReachable(); } else { trc->callback(trc, thing, kind); } goto out; } ?
|
|
||
Comment 16•15 years ago
|
||
(In reply to comment #14) > Created an attachment (id=430139) [details] > mark objects added through rt->gcExtraRootsTraceOp as gray (in addition to the > standard mark color, black). Currently if the normal GC marking starts, it has to be followed by a sweep since this will invalidate the mark bits left from the previous GC. Those bits are used to decide whether to run the finalizer during the sweep phase. So if we are going to skip the sweep for the CC, we must either restore the bits after the CC or use different bits.
|
|
||
Comment 17•15 years ago
|
||
(In reply to comment #16) > Currently if the normal GC marking starts, it has to be followed by a sweep > since this will invalidate the mark bits left from the previous GC. Hm, this is only true for doubles. For objects this is not true since the finalizer uses the free list membership to check if the GC cell is free or contains to be finalized object.
|
|
Assignee | |
Comment 21•15 years ago
|
||
added an assert
Attachment #430179 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 22•15 years ago
|
||
MarkRoots() tries to walk dead objects. That can't be good. #0 0x00007fff86adefe6 in __kill () #1 0x0000000100014720 in RealBreak () at ../../../xpcom/base/nsDebugImpl.cpp:415 #2 0x0000000100014733 in Break (aMsg=0x7fff5fbf5900 "###!!! ASSERTION: attempting to traverse dead JS Object: 'JS_IsAboutToBeFinalized(cx, p)', file ../../../../../js/src/xpconnect/src/nsXPConnect.cpp, line 698") at ../../../xpcom/base/nsDebugImpl.cpp:515 #3 0x0000000100014cb3 in NS_DebugBreak_P (aSeverity=1, aStr=0x1014fa450 "attempting to traverse dead JS Object", aExpr=0x1014fa430 "JS_IsAboutToBeFinalized(cx, p)", aFile=0x1014f9978 "../../../../../js/src/xpconnect/src/nsXPConnect.cpp", aLine=698) at ../../../xpcom/base/nsDebugImpl.cpp:360 #4 0x000000010004c75e in nsXPConnect::Traverse (this=0x10433cb30, p=0x11d97c300, cb=@0x7fff5fbf5fc0) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:698 #5 0x0000000101248623 in GCGraphBuilder::Traverse (this=0x7fff5fbf5fc0, aPtrInfo=0x115d33af0) at ../../../xpcom/base/nsCycleCollector.cpp:1476 #6 0x00000001012486be in nsCycleCollector::MarkRoots (this=0x104896000, builder=@0x7fff5fbf5fc0) at ../../../xpcom/base/nsCycleCollector.cpp:1698 #7 0x0000000101248bac in nsCycleCollector::BeginCollection (this=0x104896000) at ../../../xpcom/base/nsCycleCollector.cpp:2645 #8 0x0000000101248c20 in nsCycleCollector_beginCollection () at ../../../xpcom/base/nsCycleCollector.cpp:3235 #9 0x000000010004d3cc in XPCCycleCollectGCCallback (cx=0x10508f000, status=JSGC_MARK_END) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:388 #10 0x0000000103900261 in js_GC (cx=0x10508f000, gckind=GC_NORMAL) at ../../../js/src/jsgc.cpp:3184 #11 0x000000010389e1e7 in JS_GC (cx=0x10508f000) at ../../../js/src/jsapi.cpp:2294 #12 0x000000010004d2f1 in nsXPConnect::Collect (this=0x10433cb30) at ../../../../../js/src/xpconnect/src/nsXPConnect.cpp:461 #13 0x0000000101248d61 in nsCycleCollector::Collect (this=0x104896000, aTryCollections=5) at ../../../xpcom/base/nsCycleCollector.cpp:2522 #14 0x0000000101248efb in nsCycleCollector::Shutdown (this=0x104896000) at ../../../xpcom/base/nsCycleCollector.cpp:2754 #15 0x0000000101248f2b in nsCycleCollector_shutdown () at ../../../xpcom/base/nsCycleCollector.cpp:3257 #16 0x00000001011cea8e in mozilla::ShutdownXPCOM (servMgr=0x0) at ../../../xpcom/build/nsXPComInit.cpp:824 #17 0x00000001011ced57 in NS_ShutdownXPCOM_P (servMgr=0x10410b7c8) at ../../../xpcom/build/nsXPComInit.cpp:728 #18 0x0000000100020514 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0x7fff5fbfe660) at ../../../toolkit/xre/nsAppRunner.cpp:1055 #19 0x0000000100027157 in XRE_main (argc=1, argv=0x7fff5fbfea30, aAppData=0x104300080) at ../../../toolkit/xre/nsAppRunner.cpp:3533 #20 0x00000001000011fd in main (argc=1, argv=0x7fff5fbfea30) at ../../../browser/app/nsBrowserApp.cpp:158
|
|
Assignee | |
Comment 23•15 years ago
|
||
Patch partially debugging with igor. No longer crashes, but still leaks.
Attachment #430180 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 24•15 years ago
|
||
We leak a bunch of stuff at shutdown, but no Window object. This smells like an existing but to me that the new interaction with the GC exposes. We no longer automatically drop JSObjects as roots. Instead, we rely on the wrapper being Unlink()-ed. If that's missing, the cycle remains intact. Leaked URLs: file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/MinefieldDebug.app/Contents/MacOS/chrome/browser.jar file:///Users/gal/workspace/tracemonkey-repository/debug-build/dist/MinefieldDebug.app/Contents/MacOS/chrome/toolkit.jar http://screwdriver/ http://screwdriver/ http://test.bclary.com/ http://test.bclary.com/ https://bug494186.bugzilla.mozilla.org/ https://bug494186.bugzilla.mozilla.org/ resource://gre-resources/ua.css resource://gre-resources/html.css chrome://global/content/bindings/videocontrols.xml#videoControls chrome://xbl-marquee/content/xbl-marquee.xml#marquee-horizontal chrome://xbl-marquee/content/xbl-marquee.xml#marquee-vertical chrome://global/content/xul.css chrome://global/content/bindings/general.xml#root-element chrome://global/content/bindings/button.xml#button chrome://global/content/bindings/button.xml#button-periodic-redraw chrome://global/content/bindings/button.xml#button-repeat chrome://global/content/bindings/button.xml#menu chrome://global/content/bindings/button.xml#menu-button chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton chrome://global/content/bindings/toolbarbutton.xml#menu chrome://global/content/bindings/toolbarbutton.xml#menu-button chrome://global/content/bindings/browser.xml#browser chrome://global/content/bindings/editor.xml#editor chrome://global/content/bindings/general.xml#iframe chrome://global/content/bindings/notification.xml#notificationbox chrome://global/content/bindings/notification.xml#notification chrome://global/content/bindings/general.xml#image chrome://global/content/bindings/checkbox.xml#checkbox chrome://global/content/bindings/radio.xml#radiogroup chrome://global/content/bindings/radio.xml#radio chrome://global/content/bindings/groupbox.xml#groupbox chrome://global/content/bindings/groupbox.xml#caption chrome://global/content/bindings/toolbar.xml#toolbox chrome://global/content/bindings/toolbar.xml#toolbar chrome://global/content/bindings/toolbar.xml#toolbardecoration chrome://global/content/bindings/toolbar.xml#toolbardecoration chrome://global/content/bindings/toolbar.xml#toolbardecoration chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem-palette chrome://global/content/bindings/toolbar.xml#menubar chrome://global/content/bindings/menu.xml#menu-menubar chrome://global/content/bindings/menu.xml#menu-menubar-iconic chrome://global/content/bindings/menu.xml#menu chrome://global/content/bindings/menu.xml#menu-iconic chrome://global/content/bindings/menu.xml#menuitem chrome://global/content/bindings/menu.xml#menuitem-iconic chrome://global/content/bindings/menu.xml#menuitem-iconic-desc-noaccel chrome://global/content/bindings/menu.xml#menuitem-iconic chrome://global/content/bindings/menu.xml#menubutton-item chrome://global/content/bindings/menu.xml#menuseparator chrome://global/content/bindings/popup.xml#popup chrome://global/content/bindings/popup.xml#panel chrome://global/content/bindings/popup.xml#tooltip chrome://global/content/bindings/listbox.xml#listbox chrome://global/content/bindings/listbox.xml#listhead chrome://global/content/bindings/listbox.xml#listrows chrome://global/content/bindings/listbox.xml#listitem chrome://global/content/bindings/listbox.xml#listitem-checkbox chrome://global/content/bindings/listbox.xml#listheader chrome://global/content/bindings/listbox.xml#listcell chrome://global/content/bindings/listbox.xml#listcell-checkbox chrome://global/content/bindings/listbox.xml#listitem-iconic chrome://global/content/bindings/listbox.xml#listitem-checkbox-iconic chrome://global/content/bindings/listbox.xml#listcell-iconic chrome://global/content/bindings/listbox.xml#listcell-checkbox-iconic chrome://global/content/bindings/tree.xml#tree chrome://global/content/bindings/tree.xml#treecols chrome://global/content/bindings/tree.xml#treecol chrome://global/content/bindings/tree.xml#treecol-image chrome://global/content/bindings/tree.xml#treebody chrome://global/content/bindings/tree.xml#treerows chrome://global/content/bindings/tree.xml#columnpicker chrome://global/content/bindings/general.xml#deck chrome://global/content/bindings/tabbox.xml#tabbox chrome://global/content/bindings/tabbox.xml#tabs chrome://global/content/bindings/tabbox.xml#tab chrome://global/content/bindings/tabbox.xml#tabpanels chrome://global/content/bindings/progressmeter.xml#progressmeter chrome://global/content/bindings/text.xml#text-base chrome://global/content/bindings/text.xml#text-label chrome://global/content/bindings/text.xml#text-link chrome://global/content/bindings/text.xml#label-control chrome://global/content/bindings/textbox.xml#textbox chrome://global/content/bindings/textbox.xml#textarea chrome://global/content/bindings/textbox.xml#input-box chrome://global/content/bindings/textbox.xml#input-box-spell chrome://global/content/bindings/textbox.xml#timed-textbox chrome://global/content/bindings/textbox.xml#search-textbox chrome://global/content/bindings/numberbox.xml#numberbox chrome://global/content/bindings/autocomplete.xml#autocomplete chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup chrome://global/content/bindings/autocomplete.xml#autocomplete-tree chrome://global/content/bindings/autocomplete.xml#autocomplete-treebody chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem chrome://global/content/bindings/autocomplete.xml#autocomplete-treerows chrome://global/content/bindings/autocomplete.xml#history-dropmarker chrome://global/content/bindings/colorpicker.xml#colorpicker chrome://global/content/bindings/colorpicker.xml#colorpicker-button chrome://global/content/bindings/colorpicker.xml#colorpickertile chrome://global/content/bindings/menulist.xml#menulist chrome://global/content/bindings/menulist.xml#menulist-editable chrome://global/content/bindings/menulist.xml#menulist-description chrome://global/content/bindings/menu.xml#menuitem-iconic-noaccel chrome://global/content/bindings/general.xml#dropmarker chrome://global/content/bindings/resizer.xml#resizer chrome://global/content/bindings/splitter.xml#splitter chrome://global/content/bindings/splitter.xml#grippy chrome://global/content/bindings/scrollbar.xml#thumb chrome://global/content/bindings/scale.xml#scalethumb chrome://global/content/bindings/scale.xml#scale chrome://global/content/bindings/scale.xml#scaleslider chrome://global/content/bindings/scrollbox.xml#scrollbox chrome://global/content/bindings/scrollbox.xml#arrowscrollbox chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll chrome://global/content/bindings/scrollbox.xml#autorepeatbutton chrome://global/content/bindings/general.xml#statusbar chrome://global/content/bindings/general.xml#statusbarpanel chrome://global/content/bindings/general.xml#statusbarpanel-iconic chrome://global/content/bindings/general.xml#statusbarpanel-iconic-text chrome://global/content/bindings/general.xml#statusbarpanel-menu-iconic chrome://global/content/bindings/spinbuttons.xml#spinbuttons chrome://global/content/bindings/stringbundle.xml#stringbundleset chrome://global/content/bindings/stringbundle.xml#stringbundle chrome://global/content/bindings/dialog.xml#dialog chrome://global/content/bindings/dialog.xml#dialogheader chrome://global/content/bindings/wizard.xml#wizard chrome://global/content/bindings/wizard.xml#wizardpage chrome://global/content/bindings/wizard.xml#wizard-header chrome://global/content/bindings/wizard.xml#wizard-buttons chrome://global/content/bindings/preferences.xml#prefwindow chrome://global/content/bindings/preferences.xml#prefpane chrome://global/content/bindings/preferences.xml#preferences chrome://global/content/bindings/preferences.xml#preference chrome://global/content/bindings/preferences.xml#panebutton chrome://global/content/bindings/expander.xml#expander chrome://global/content/bindings/richlistbox.xml#richlistbox chrome://global/content/bindings/richlistbox.xml#richlistitem chrome://global/content/bindings/datetimepicker.xml#datepicker chrome://global/content/bindings/datetimepicker.xml#datepicker-popup chrome://global/content/bindings/datetimepicker.xml#datepicker-grid chrome://global/content/bindings/datetimepicker.xml#timepicker chrome://global/content/bindings/findbar.xml#findbar chrome://global/content/bindings/findbar.xml#findbar-textbox chrome://global/content/bindings/filefield.xml#filefield resource://gre-resources/quirk.css chrome://global/skin/scrollbars.css chrome://global/content/bindings/scrollbar.xml#scrollbar chrome://global/content/bindings/scrollbar.xml#scrollbar-base chrome://global/content/bindings/scrollbar.xml#scrollbar resource://gre-resources/forms.css chrome://global/content/platformHTMLBindings.xml#inputFields chrome://global/content/platformHTMLBindings.xml#textAreas resource://gre-resources/arrow.gif resource://gre-resources/arrowd.gif chrome://mozapps/skin/places/defaultFavicon.png chrome://global/content/bindings/general.xml x:///content/global/bindings/general.xml chrome://global/content/bindings/general.xml chrome://global/content/bindings/general.xml#basecontrol chrome://global/content/bindings/general.xml chrome://global/content/bindings/general.xml#basetext chrome://global/content/bindings/general.xml#control-item chrome://global/content/bindings/general.xml#root-element chrome://global/content/bindings/general.xml#iframe chrome://global/content/bindings/general.xml#statusbarpanel chrome://global/content/bindings/general.xml#statusbarpanel-menu-iconic chrome://global/content/bindings/general.xml#statusbar chrome://global/content/bindings/general.xml#statusbar-drag chrome://global/content/bindings/general.xml#statusbarpanel-iconic chrome://global/content/bindings/general.xml#statusbarpanel-iconic-text chrome://global/content/bindings/general.xml#image chrome://global/content/bindings/general.xml#deck chrome://global/content/bindings/general.xml#dropmarker chrome://global/content/bindings/general.xml#windowdragbox chrome://global/content/bindings/popup.xml x:///content/global/bindings/popup.xml chrome://global/content/bindings/popup.xml chrome://global/content/bindings/popup.xml#popup-base chrome://global/content/bindings/popup.xml chrome://global/content/bindings/popup.xml#popup chrome://global/content/bindings/popup.xml#panel chrome://global/content/bindings/popup.xml#tooltip chrome://global/content/bindings/popup.xml#popup-scrollbars chrome://global/skin/popup.css chrome://global/content/bindings/stringbundle.xml x:///content/global/bindings/stringbundle.xml chrome://global/content/bindings/stringbundle.xml chrome://global/content/bindings/stringbundle.xml#stringbundleset chrome://global/content/bindings/stringbundle.xml chrome://global/content/bindings/stringbundle.xml#stringbundle chrome://global/content/bindings/toolbar.xml x:///content/global/bindings/toolbar.xml chrome://global/content/bindings/toolbar.xml chrome://global/content/bindings/toolbar.xml#toolbar-base chrome://global/content/bindings/toolbar.xml chrome://global/content/bindings/toolbar.xml#toolbox chrome://global/content/bindings/toolbar.xml#toolbar chrome://global/content/bindings/toolbar.xml#toolbar-menubar-autohide chrome://global/content/bindings/toolbar.xml#toolbar-drag chrome://global/content/bindings/toolbar.xml#menubar chrome://global/content/bindings/toolbar.xml#toolbardecoration chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem chrome://global/content/bindings/toolbar.xml#toolbarpaletteitem-palette chrome://global/skin/toolbar.css chrome://global/skin/toolbar/toolbar-separator.png chrome://global/skin/10pct_transparent_grey.png chrome://global/skin/toolbar/spring.png chrome://global/skin/10pct_transparent_grey.png chrome://browser/content/urlbarBindings.xml x:///content/browser/urlbarBindings.xml chrome://browser/content/urlbarBindings.xml chrome://browser/content/urlbarBindings.xml#urlbar chrome://browser/content/urlbarBindings.xml chrome://browser/content/urlbarBindings.xml#browser-autocomplete-result-popup chrome://browser/content/urlbarBindings.xml#urlbar-rich-result-popup chrome://global/content/bindings/autocomplete.xml x:///content/global/bindings/autocomplete.xml chrome://global/content/bindings/autocomplete.xml chrome://global/content/bindings/autocomplete.xml#autocomplete chrome://global/content/bindings/autocomplete.xml chrome://global/content/bindings/autocomplete.xml#autocomplete-result-popup chrome://global/content/bindings/autocomplete.xml#autocomplete-base-popup chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem chrome://global/content/bindings/autocomplete.xml#autocomplete-tree chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistbox chrome://global/content/bindings/autocomplete.xml#autocomplete-treebody chrome://global/content/bindings/autocomplete.xml#autocomplete-treerows chrome://global/content/bindings/autocomplete.xml#history-dropmarker chrome://global/skin/tree.css chrome://global/skin/tree/sort-asc.gif chrome://global/skin/tree/sort-dsc.gif chrome://global/skin/tree/columnpicker.gif chrome://global/skin/autocomplete.css chrome://global/skin/icons/autocomplete-dropmarker.png chrome://browser/content/tabbrowser.xml x:///content/browser/tabbrowser.xml chrome://browser/content/tabbrowser.xml chrome://browser/content/tabbrowser.xml#tabbrowser chrome://browser/content/tabbrowser.xml chrome://browser/content/tabbrowser.xml#tabbrowser-arrowscrollbox chrome://browser/content/tabbrowser.xml#tabbrowser-tabs chrome://browser/content/tabbrowser.xml#tabbrowser-close-tab-button chrome://browser/content/tabbrowser.xml#tabbrowser-tab chrome://browser/content/tabbrowser.xml#tabbrowser-alltabs-popup chrome://browser/content/tabbrowser.css chrome://browser/content/tabbrowser.xml#tabbrowser-tabs chrome://browser/content/tabbrowser.xml#tabbrowser-tab chrome://browser/content/tabbrowser.xml#tabbrowser-arrowscrollbox chrome://browser/content/tabbrowser.xml#tabbrowser-alltabs-popup chrome://browser/content/tabbrowser.xml#tabbrowser-close-tab-button chrome://global/content/bindings/tabbox.xml x:///content/global/bindings/tabbox.xml chrome://global/content/bindings/tabbox.xml chrome://global/content/bindings/tabbox.xml#tab-base chrome://global/content/bindings/tabbox.xml chrome://global/content/bindings/tabbox.xml#tabbox chrome://global/content/bindings/tabbox.xml#tabs chrome://global/content/bindings/tabbox.xml#tabpanels chrome://global/content/bindings/tabbox.xml#tab chrome://global/skin/tabbox.css chrome://global/content/bindings/scrollbox.xml x:///content/global/bindings/scrollbox.xml chrome://global/content/bindings/scrollbox.xml chrome://global/content/bindings/scrollbox.xml#scrollbox-base chrome://global/content/bindings/scrollbox.xml chrome://global/content/bindings/scrollbox.xml#scrollbox chrome://global/content/bindings/scrollbox.xml#arrowscrollbox chrome://global/content/bindings/scrollbox.xml#autorepeatbutton chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll chrome://global/skin/scrollbox.css chrome://global/skin/arrow/arrow-lft-sharp.gif chrome://global/skin/arrow/arrow-rit-sharp.gif chrome://global/skin/arrow/arrow-lft-dis.gif chrome://global/skin/arrow/arrow-rit-dis.gif chrome://global/skin/arrow/arrow-up-sharp.gif chrome://global/skin/arrow/arrow-dn-sharp.gif chrome://global/skin/arrow/arrow-up-dis.gif chrome://global/skin/arrow/arrow-dn-dis.gif chrome://global/skin/menu/menu-arrow.png chrome://global/content/bindings/toolbarbutton.xml x:///content/global/bindings/toolbarbutton.xml chrome://global/content/bindings/toolbarbutton.xml chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton chrome://global/content/bindings/toolbarbutton.xml chrome://global/content/bindings/toolbarbutton.xml#menu chrome://global/content/bindings/toolbarbutton.xml#menu-vertical chrome://global/content/bindings/toolbarbutton.xml#menu-button chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image chrome://global/skin/toolbarbutton.css chrome://global/skin/arrow/arrow-dn.png chrome://global/skin/arrow/arrow-dn-dis.png chrome://global/skin/arrow/arrow-dn.png chrome://global/skin/arrow/arrow-dn-dis.png chrome://global/content/bindings/button.xml x:///content/global/bindings/button.xml chrome://global/content/bindings/button.xml chrome://global/content/bindings/button.xml#button-base chrome://global/content/bindings/button.xml chrome://global/content/bindings/button.xml#button chrome://global/content/bindings/button.xml#button-periodic-redraw chrome://global/content/bindings/button.xml#menu chrome://global/content/bindings/button.xml#menu-button-base chrome://global/content/bindings/button.xml#menu-button chrome://global/content/bindings/button.xml#button-image chrome://global/content/bindings/button.xml#button-repeat chrome://global/content/bindings/text.xml x:///content/global/bindings/text.xml chrome://global/content/bindings/text.xml chrome://global/content/bindings/text.xml#text-base chrome://global/content/bindings/text.xml chrome://global/content/bindings/text.xml#text-label chrome://global/content/bindings/text.xml#label-control chrome://global/content/bindings/text.xml#text-link chrome://global/skin/dropmarker.css chrome://global/skin/arrow/arrow-dn.gif chrome://global/skin/arrow/arrow-dn-dis.gif chrome://global/content/bindings/notification.xml x:///content/global/bindings/notification.xml chrome://global/content/bindings/notification.xml chrome://global/content/bindings/notification.xml#notificationbox chrome://global/content/bindings/notification.xml chrome://global/content/bindings/notification.xml#notification chrome://global/content/bindings/browser.xml x:///content/global/bindings/browser.xml chrome://global/content/bindings/browser.xml chrome://global/content/bindings/browser.xml#browser chrome://global/content/bindings/browser.xml chrome://global/content/bindings/findbar.xml x:///content/global/bindings/findbar.xml chrome://global/content/bindings/findbar.xml chrome://global/content/bindings/findbar.xml#findbar-textbox chrome://global/content/bindings/findbar.xml chrome://global/content/bindings/findbar.xml#findbar chrome://global/skin/findBar.css chrome://global/skin/icons/closetab.png chrome://global/skin/icons/closetab-hover.png chrome://global/skin/icons/closetab-active.png chrome://global/skin/icons/checkbox.png chrome://global/skin/icons/search-textbox.png chrome://global/content/bindings/progressmeter.xml x:///content/global/bindings/progressmeter.xml chrome://global/content/bindings/progressmeter.xml chrome://global/content/bindings/progressmeter.xml#progressmeter chrome://global/content/bindings/progressmeter.xml chrome://global/content/bindings/progressmeter.xml#progressmeter-undetermined chrome://global/content/bindings/progressmeter.xml#progressmeter-periodic-redraw chrome://global/skin/progressmeter.css chrome://global/content/bindings/textbox.xml x:///content/global/bindings/textbox.xml chrome://global/content/bindings/textbox.xml chrome://global/content/bindings/textbox.xml#textbox chrome://global/content/bindings/textbox.xml chrome://global/content/bindings/textbox.xml#timed-textbox chrome://global/content/bindings/textbox.xml#search-textbox chrome://global/content/bindings/textbox.xml#textarea chrome://global/content/bindings/textbox.xml#input-box chrome://global/content/bindings/textbox.xml#input-box-spell chrome://global/content/textbox.css chrome://global/skin/textbox.css chrome://global/content/autocomplete.css chrome://global/skin/icons/searchfield-regular-cancel.png chrome://global/skin/icons/searchfield-small-cancel.png chrome://browser/content/places/toolbar.xml x:///content/browser/places/toolbar.xml chrome://browser/content/places/toolbar.xml chrome://browser/content/places/toolbar.xml#places-bar chrome://browser/content/places/toolbar.xml chrome://browser/skin/places/places.css chrome://global/skin/icons/menulist-dropmarker.png chrome://mozapps/skin/places/defaultFavicon.png chrome://browser/skin/places/livemarkItem.png chrome://global/skin/tree/folder.png chrome://browser/skin/places/allBookmarks.png chrome://browser/skin/page-livemarks.png chrome://browser/skin/places/bookmarksToolbar.png chrome://browser/skin/places/bookmarksMenu.png chrome://browser/skin/places/unfiledBookmarks.png chrome://browser/skin/places/query.png chrome://mozapps/skin/places/tagContainerIcon.png chrome://browser/skin/places/history.png chrome://global/skin/tree/folder.png chrome://global/skin/tree/folder.png chrome://browser/skin/places/history.png chrome://global/skin/tree/folder.png chrome://browser/content/places/menu.xml x:///content/browser/places/menu.xml chrome://browser/content/places/menu.xml chrome://browser/content/places/menu.xml#places-popup-base chrome://browser/content/places/menu.xml chrome://browser/content/places/menu.xml#places-menupopup chrome://global/skin/button.css chrome://global/skin/icons/question-mark.png chrome://browser/content/search/search.xml x:///content/browser/search/search.xml chrome://browser/content/search/search.xml chrome://browser/content/search/search.xml#searchbar chrome://browser/content/search/search.xml chrome://browser/content/search/search.xml#searchbar-textbox chrome://browser/content/search/searchbarBindings.css chrome://browser/content/search/search.xml#searchbar-textbox chrome://browser/skin/searchbar.css chrome://mozapps/skin/places/defaultFavicon.png chrome://browser/skin/Search-addengines.png chrome://global/skin/arrow/arrow-dn.png chrome://browser/skin/Search.png chrome://global/content/bindings/menu.xml x:///content/global/bindings/menu.xml chrome://global/content/bindings/menu.xml chrome://global/content/bindings/menu.xml#menuitem-base chrome://global/content/bindings/menu.xml chrome://global/content/bindings/menu.xml#menu-base chrome://global/content/bindings/menu.xml#menu chrome://global/content/bindings/menu.xml#menuitem chrome://global/content/bindings/menu.xml#menu-menubar chrome://global/content/bindings/menu.xml#menu-menubar-iconic chrome://global/content/bindings/menu.xml#menuitem-iconic chrome://global/content/bindings/menu.xml#menuitem-iconic-noaccel chrome://global/content/bindings/menu.xml#menuitem-iconic-desc-noaccel chrome://global/content/bindings/menu.xml#menu-iconic chrome://global/content/bindings/menu.xml#menubutton-item chrome://global/content/bindings/menu.xml#menuseparator chrome://global/skin/menu.css chrome://global/skin/menu/menu-arrow.png chrome://global/skin/menu/menu-check.png chrome://global/skin/menu/menu-check.png chrome://global/skin/columnselect.gif resource://gre-resources/loading-image.png resource://gre-resources/broken-image.png resource://gre-resources/loading-image.png resource://gre-resources/broken-image.png
|
|
Assignee | |
Comment 25•15 years ago
|
||
Here is an example: The CC has identified a bunch of cycles and is unlinking them. It unlinks the JSObject part of a wrapper first. That goes into nsXPConnect::RootAndUnlinkJSObjects(), where we don't do anything. unlink: object 0x11e6acd00 class 0x11f638b38 Window properties: proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0x11e625bc0> parent null private 0x11f638cf0 slots: We will find the same native wrapper being held here: unlink: object 0x11e686cc0 class 0x101d479e0 XPCNativeWrapper properties: proto null parent <Window object at 0x11e6acd00> private 0x11f638cf0 slots: 3 (reserved) = 2 Here again we don't do anything. Eventually we Unlink the wrapper via XPCWrappedNative::RootAndUnlinkJSObject(p), with p being the native wrapper, not a JSObject as the name would suggest: expirewrapper: 1f638cf0 So for this window object this works as expected. However, for this particular ChromeWindow object this doesn't work as expected: unlink: object 0x11e6f0300 class 0x11e8484f8 ChromeWindow properties: proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0x11e6f0780> parent null private 0x1043c1a50 We never see the wrapper (0x1043c1a50) being expired. Oh wrapper expiration callback, Where Art Thou?
|
|
Assignee | |
Comment 26•15 years ago
|
||
Ok, bz thinks that the ChromeWindow goes into AddJSHolder(), which always QIs to an nsXPCOMCycleCollectionParticipant, which again implements an empty RootAndUnlinkJSObjects(). I think that's a bug of some sort. Maybe Blake can sort it out. In the meantime, it should be perfectly safe (and perfectly ugly) to reach from the JSObject into the wrapper and expire it from there.
|
||
Comment 27•15 years ago
|
||
What goes into AddJSHolder are elements, not windows, but the former can of course entrain the latter.
|
||
Comment 28•15 years ago
|
||
(In reply to comment #0) > A bit of archeology shows that way back we decided to intertwine GC and CC to > avoid a redundant scan/mark. That's basically exactly what I propose we try > now, since we can make it concurrent and interruptible. We will need some > experimenting with this to see whether comment #1 is faster, or the decision > back in 2007 was right after all. Doesn't look super hard to hack that up > though (at least to he point where we can benchmark it), since its mostly > undoing changes. > https://bugzilla.mozilla.org/show_bug.cgi?id=377884#c9 The current dance between GC and CC mostly came about in bug 379718 comment 19, with followups in bug 401687. At the time it was a significant improvement to CC times, because it avoided duplicating work between the two traversals of the JS heap (one for the GC with a callback to calculate real refcounts and a partial one for the CC to discover cycles). It might block us from doing other improvements, but it did improve the performance of the CC (I know, it's hard to imagine it used to be worse). (In reply to comment #8) > Cycles involving JS Objects are not immediately collected. > Since cycles are long-lived, this mild delay until the cycle gets collected > should be immaterial. We'll have to make sure that this doesn't keep traversing collectable cycles in the next CC. I'm not really sure about how this works with wrappers. If the JS object is kept alive until the next CC but its wrapper is expired, won't we risk handing out an expired wrapper if somebody asks for it? (In reply to comment #26) > Ok, bz thinks that the ChromeWindow goes into AddJSHolder(), which always QIs > to an nsXPCOMCycleCollectionParticipant, which again implements an empty > RootAndUnlinkJSObjects(). I think that's a bug of some sort. Not really sure what you mean, RootAndUnlinkJSObjects is virtual. nsXPCOMCycleCollectionParticipant::RootAndUnlinkJSObjects is empty because most objects don't hold JS objects. What code passes an object that holds a JS object but has an empty RootAndUnlinkJSObjects in its CC participant to AddJSHolder?
|
|
||
Comment 29•15 years ago
|
||
RootAndUnlinkJSObject might be badly named, it really means root the object being unlinked (which boils down to AddRef'ing C++ objects) and make that object forget about any JS objects it's holding. We needed to combine these two operations. We needed to make sure C++ objects don't have references to JS objects that are swept. And we needed to keep C++ objects alive for unlinking across the sweep phase, since sweeping JS objects used to actually release some C++ objects. So we root C++ objects, unlink their edges to JS objects, sweep JS objects, unlink C++ objects, and unroot C++ objects. AFAICT in the new world we might be able to move all unlinking back into Unlink.
|
|
Assignee | |
Comment 30•15 years ago
|
||
The principal problem is that we currently ignore most Unlink() operations on XPCOM objects such as wrappers. Most of them end up in nsXPConnect::RootAndUnlinkJSObject, which is empty, instead of XPCWrappedNative::_cycleCollectorGlobal::RootAndUnlinkJSObject as one would expect. This has to do with the way we select CycleCollectorParticipants. As a result we currently rely on JSObjects to go dead to break the cycle. That exactly is what the patch in this bug no longer does since we are separated from the GC cycle. I wanted to rely on Unlink of XPCOM objects, but that seems far fetched at this point (a lot of different things to implement Unlink for). Instead I will make sure objects the CC declares dead are not marked during the next GC run.
|
|
Assignee | |
Comment 31•15 years ago
|
||
Unlink JSObjects by entering them into a "dead objects" hash set that persists until the next GC.
Attachment #430208 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 34•15 years ago
|
||
remove bogus asserts, DEBUG_CC should work again
Attachment #430273 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 37•14 years ago
|
||
refresh, clear weak roots when we are about to cc
Attachment #446001 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 38•14 years ago
|
||
This looks good. No more shutdown leaks in my local testing. Send the patch to the try server.
|
||
Comment 39•14 years ago
|
||
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1276654212.1276654987.17830.gz&fulltext=1
|
|
||
Comment 41•14 years ago
|
||
Actually, it looks like we crash sometimes, see for example: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1276657700.1276657962.30057.gz&fulltext=1#err0 Also, it didn't build on Linux and Windows because of missing JS_FRIEND_API on js_IsAboutToBeFinalized, js_SetMarkColor and js_GCThingIsMarked in jsgc.h
|
|
Assignee | |
Comment 42•14 years ago
|
||
I don't think the crash is us. That looks like a current botch in TM tip (I think). Any ideas on the remaining leak? It doesn't show for startup/shutdown any more for me.
|
|
||
Comment 43•14 years ago
|
||
I've pushed this on an almost tip trunk tree to tryserver, so far only known orange on Linux but no unknown leaks. Still need to wait for the other platforms. It's probably a bad idea to push patches to tryserver that have DEBUG_CC defined.
|
|
Assignee | |
Updated•14 years ago
|
Attachment #451561 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•14 years ago
|
Attachment #451415 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•14 years ago
|
Summary: disentangle cycle collector and JS GC, if feasible → disentangle cycle collector and JS GC
|
|
Assignee | |
Comment 46•14 years ago
|
||
Review ping. Patch passes try server. Blocking the rest of my CC patch queue.
|
|
Assignee | |
Comment 47•14 years ago
|
||
Review ping. I need to land this soon, it blocks a bunch of GC refactoring work.
|
|
||
Comment 48•14 years ago
|
||
Comment on attachment 451561 [details] [diff] [review] clean patch >diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h >@@ -1662,23 +1663,20 @@ struct JSContext >- /* Flag to indicate that we run inside gcCallback(cx, JSGC_MARK_END). */ >- JSPackedBool insideGCMarkCallback; I don't understand why you can remove this, please get review on this from a JS peer. >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp >+IsMarkedGCThing(void *thing, uint32 color) > { > size_t index; > jsbitmap *markBitmap = GetGCThingMarkBit(thing, index); >- return !!JS_TEST_BIT(markBitmap, index); >+ return !!JS_TEST_BIT(markBitmap, index + color); >+MarkIfUnmarkedGCThing(void *thing, uint32 color) > { > size_t index; > jsbitmap *markBitmap = GetGCThingMarkBit(thing, index); > if (JS_TEST_BIT(markBitmap, index)) > return false; > JS_SET_BIT(markBitmap, index); >+ if (color) >+ JS_SET_BIT(markBitmap, index + color); Shouldn't these verify (assert) that color is allowed for thing? >+JS_FRIEND_API(bool) >+js_GCThingIsMarked(void *thing, uint32 color) >+{ >+ JS_ASSERT(color != BLACK); Not sure why we would disallow BLACK? >diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp >@@ -389,42 +384,33 @@ nsXPConnect::Collect() >+ // Phase: Phases > // > // 1. marking of the roots in category 1 by having the JS GC do its marking >+ // 2. marking of the roots in category 2 by XPCJSRuntime::TraceXPConnectRoots >+ // using an additional color (gray). >+ // 3. end of GC, GC can sweep its heap >+ // 4. walk gray objects and add them to the cycle collector, cycle collect Maybe clarify that JS objects that become unreachable from the cycle collection will be swept by the next GC? >@@ -668,25 +650,36 @@ WrapperIsNotMainThreadOnly(XPCWrappedNat > nsXPConnect::Traverse(void *p, nsCycleCollectionTraversalCallback &cb) > { > if(!mCycleCollectionContext) > return NS_ERROR_FAILURE; > > JSContext *cx = mCycleCollectionContext->GetJSContext(); > > uint32 traceKind = js_GetGCThingTraceKind(p); >+ if(!ADD_TO_CC(traceKind)) >+ return NS_OK; I don't think we can do this. What prevents anyone from adding a non-JSObject to the cycle collector as a root? I think we should move this check to nsXPConnect::ToParticipant, ie return null there if !ADD_TO_CC(js_GetGCThingTraceKind(p)) and make this one an assertion. That should prevent JS objects other than JSObjects from being added to the CC. BTW, using GRAY for JSTRACE_XML is ok? >@@ -924,16 +913,19 @@ nsXPConnect::GetRequestDepth(JSContext* > class JSContextParticipant : public nsCycleCollectionParticipant > { > public: > NS_IMETHOD RootAndUnlinkJSObjects(void *n) > { >+ JSContext *cx = static_cast<JSContext*>(n); >+ NS_ASSERTION(cx->globalObject, "global object NULL before unlinking"); >+ cx->globalObject = nsnull; This looks a bit scary. I think we didn't do it before because there's no real order to unlinking, let's hope nothing uses globalObject during unlinking. >diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp >+static const uint32 GRAY = 1; Can we at least share this with nsXPConnect? Having the same value defined in multiple files is annoying. >+void >+XPCJSRuntime::ClearWeakRoots() > { >- mUnrootedGlobalCount = 0; > JSContext *iter = nsnull, *acx; >+ > while((acx = JS_ContextIterator(GetJSRuntime(), &iter))) > { >- NS_ASSERTION(!JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL), >- "unrooted global should be set only during CC"); > if(XPCPerThreadData::IsMainThread(acx) && >- nsXPConnect::GetXPConnect()->GetRequestDepth(acx) == 0) >- { >+ nsXPConnect::GetXPConnect()->GetRequestDepth(acx) == 0) { { goes on its own line in XPConnect. > JS_ClearNewbornRoots(acx); >- if(acx->globalObject) >- { >- JS_ToggleOptions(acx, JSOPTION_UNROOTED_GLOBAL); >- ++mUnrootedGlobalCount; >- } >@@ -500,16 +460,22 @@ JSBool XPCJSRuntime::GCCallback(JSContex >+ // We always want cx->globalObject to be unrooted. >+ JSContext *iter = nsnull, *acx; >+ while ((acx = JS_ContextIterator(self->GetJSRuntime(), &iter))) { { goes on its own line in XPConnect. >+ if (!JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)) >+ JS_ToggleOptions(acx, JSOPTION_UNROOTED_GLOBAL); >+ } Why not keep the toggling of JSOPTION_UNROOTED_GLOBAL and the clearing of the weak roots combined, and call them from the XPCJSRuntime::GCCallback? Afaik clearing weak roots is a good thing even if we don't cycle collect, so it makes sense to do it in this callback. >diff --git a/js/src/xpconnect/src/xpcprivate.h b/js/src/xpconnect/src/xpcprivate.h >@@ -469,16 +469,18 @@ public: >+ static JSBool IsGray(void *thing); Add a comment explaining what this means. I know there's not many comments in this file, but still. >diff --git a/js/src/xpconnect/src/xpcvariant.cpp b/js/src/xpconnect/src/xpcvariant.cpp >@@ -102,19 +102,20 @@ void XPCTraceableVariant::TraceJS(JSTrac >- if(JSVAL_IS_OBJECT(tmp->mJSVal)) >- cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, >- JSVAL_TO_OBJECT(tmp->mJSVal)); >+ if(JSVAL_IS_OBJECT(tmp->mJSVal)) { >+ JSObject *obj = JSVAL_TO_OBJECT(tmp->mJSVal); >+ cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, obj); >+ } Unnecessary change. >diff --git a/js/src/xpconnect/src/xpcwrappedjs.cpp b/js/src/xpconnect/src/xpcwrappedjs.cpp >@@ -69,21 +69,22 @@ NS_CYCLE_COLLECTION_CLASSNAME(nsXPCWrapp >- if(refcnt > 1) >+ if(refcnt > 1) { > // nsXPCWrappedJS roots its mJSObj when its refcount is > 1, see > // the comment above nsXPCWrappedJS::AddRef. > cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, > tmp->GetJSObject()); >+ } Unnecessary change. >diff --git a/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp >@@ -95,18 +95,19 @@ NS_CYCLE_COLLECTION_CLASSNAME(XPCWrapped >- if(NS_SUCCEEDED(rv)) >+ if(NS_SUCCEEDED(rv)) { > cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT, obj); >+ } Unnecessary change.
Attachment #451561 -
Flags: review?(peterv) → review+
|
|
Assignee | |
Comment 49•14 years ago
|
||
> > > > uint32 traceKind = js_GetGCThingTraceKind(p); > >+ if(!ADD_TO_CC(traceKind)) > >+ return NS_OK; > > I don't think we can do this. What prevents anyone from adding a non-JSObject > to the cycle collector as a root? I think we should move this check to I don't think I understand this. Only XML and OBJ can have interior pointers to anything and thus can participate in cycles. Adding a string as a CC root is a bug and should assert. > nsXPConnect::ToParticipant, ie return null there if > !ADD_TO_CC(js_GetGCThingTraceKind(p)) and make this one an assertion. That > should prevent JS objects other than JSObjects from being added to the CC. BTW, > using GRAY for JSTRACE_XML is ok? Yes, gray is safe for anything thats larger than 8 bytes, which is object and xml (and also string as it happens, but those we never mark gray).
|
|
Assignee | |
Comment 50•14 years ago
|
||
The insideGCCallback hack is no longer necessary, because we no longer "revive" (mark) objects from within the callback after GC_MARK_END. Instead, the GC already marked all the objects (we don't clear out roots, we merely mark global objects alter, but we still mark them as part of the regular GC). GC objects that were found to be in circles are disposed at the NEXT GC. Hense we no longer have to worry about incorrectly reporting the result of "IsAboutToBeFinalized" because we haven't processed delayed children yet. It can't happen any more. Makes sense?
Attachment #451561 -
Attachment is obsolete: true
Attachment #455803 -
Flags: review?(peterv)
|
|
||
Comment 51•14 years ago
|
||
Can we use insideGCCallback ifdef DEBUG to assert that no one can mark after GC_MARK_END? Or something like that, just wondering how we can do without this flag (would need #ifdef'ed setting and clearing) and still assert. /be
|
|
||
Comment 52•14 years ago
|
||
(In reply to comment #49) > > > > > > uint32 traceKind = js_GetGCThingTraceKind(p); > > >+ if(!ADD_TO_CC(traceKind)) > > >+ return NS_OK; > > > > I don't think we can do this. What prevents anyone from adding a non-JSObject > > to the cycle collector as a root? I think we should move this check to > > I don't think I understand this. Only XML and OBJ can have interior pointers to > anything and thus can participate in cycles. Adding a string as a CC root is a > bug and should assert. The point is that you're not asserting it, you're silently ignoring it at a point where it's confusing and potentially a problem (the objects are in the graph already, but you're leaving their description in the CC empty). That's why I propose moving it to ToParticipant, which avoids those objects from getting into the graph in the first place. I personally don't think I want to force the participants to do the check and rather make the CC ignore instead of assert, if you want to just make it assert that's fine by me but you'll need to make sure we don't currently do it.
|
|
||
Comment 53•14 years ago
|
||
insideGCCallback predates the CC, if it's the only remaining user then removing it is fine by me. I just don't know if it's the only user.
|
|
Assignee | |
Comment 54•14 years ago
|
||
Attachment #455803 -
Attachment is obsolete: true
Attachment #455803 -
Flags: review?(peterv)
|
|
Assignee | |
Comment 55•14 years ago
|
||
Comment on attachment 456399 [details] [diff] [review] patch Sent to try server after discussing the remaining issues with peterv. Still need a JS side r+ for the insideGCCallback flag removal.
Attachment #456399 -
Flags: review?(igor)
|
|
||
Comment 56•14 years ago
|
||
Comment on attachment 456399 [details] [diff] [review] patch >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp >+/* >+ * Live objects are marked black. How many other additional colors are available >+ * depends on the size of the GCThing. String and doubles have no extra colors. >+ * JSObjects have 3 extra colors available. >+ */ >+static const uint32 BLACK = 0; >+ >+static void >+AssertValidColor(void *thing, uint32 color) >+{ >+ JS_ASSERT_IF(color, color < JSGCArenaInfo::fromGCThing(thing)->list->thingSize / GC_CELL_SIZE); >+} >+ The comment is wrong as sizeof(JSString) == 4 words. Plus it is not compatible with fatvalues patch which would remove the doubles. Plus with fatvalues it would make sense to bump GC_CELL_SIZE to 4 words. Plus the XPConnect does not need colors for strings. So comment that currently only single extra color for object, function and xml things is supported and assert that in AssertValidColor. > inline bool >-IsMarkedGCThing(void *thing) >+IsMarkedGCThing(void *thing, uint32 color) > { Default the color here to BLACK so IsMarkedGCThing(thing) can be kept in most of the places as is. >+ AssertValidColor(thing, color); >+ > size_t index; > jsbitmap *markBitmap = GetGCThingMarkBit(thing, index); >- return !!JS_TEST_BIT(markBitmap, index); >+ return !!JS_TEST_BIT(markBitmap, index + color); > } > > inline bool >-MarkIfUnmarkedGCThing(void *thing) >+MarkIfUnmarkedGCThing(void *thing, uint32 color) > { Write comments before the function regarding the exact semantics in presence of colors and default color to black to minimize the patch. >+ AssertValidColor(thing, color); >+ > size_t index; > jsbitmap *markBitmap = GetGCThingMarkBit(thing, index); > if (JS_TEST_BIT(markBitmap, index)) > return false; > JS_SET_BIT(markBitmap, index); >+ if (color) >+ JS_SET_BIT(markBitmap, index + color); > return true; > } > > inline size_t > ThingsPerArena(size_t thingSize) > { > JS_ASSERT(!(thingSize & GC_CELL_MASK)); > JS_ASSERT(thingSize <= GC_ARENA_SIZE); >@@ -868,23 +887,46 @@ js_GetGCThingTraceKind(void *thing) > > JSRuntime * > js_GetGCThingRuntime(void *thing) > { > jsuword chunk = JSGCArena::fromGCThing(thing)->getChunk(); > return JSGCChunkInfo::fromChunk(chunk)->runtime; > } > >-bool >+JS_FRIEND_API(bool) > js_IsAboutToBeFinalized(void *thing) > { > if (JSString::isStatic(thing)) > return false; > >- return !IsMarkedGCThing(thing); >+ return !IsMarkedGCThing(thing, BLACK); >+} >+ >+static void >+MarkDelayedChildren(JSTracer *trc); >+ >+JS_FRIEND_API(uint32) >+js_SetMarkColor(JSTracer *trc, uint32 color) >+{ Comment at the function declaration that the color will only be applied to object, function and xml things. Nit: assert here that trc == rt->gcMarkingTracer. >+ JS_ASSERT(color < sizeof(JSObject)/GC_CELL_SIZE); >+ >+ /* Must process any delayed tracing here, otherwise we confuse colors. */ >+ MarkDelayedChildren(trc); >+ >+ JSRuntime *rt = trc->context->runtime; >+ uint32 oldColor = rt->gcMarkColor; Move gcMarkColor into a sublass of JSTracer that the GC uses to avoid adding to the runtime yet another flag that is relevant only during GC-marking. Also the code should assert that no color-to-color transitions should happen, i.e. either old or new color should be black. >+JS_FRIEND_API(bool) >+js_GCThingIsMarked(void *thing, uint32 color) >+{ Given that for now the only one color is supported rename this into js_IsColoredGCThing(thing). >diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp >@@ -304,23 +304,21 @@ void XPCJSRuntime::TraceJS(JSTracer* trc ... >+ >+ // Mark these roots as gray so the CC can walk them later. >+ uint32 oldColor = js_SetMarkColor(trc, XPC_GC_COLOR_GRAY); >+ self->TraceXPConnectRoots(trc); TraceJS currently is called before the conservative stack scanner scans the stack, see js_TraceRuntime. So the latter should be changed so rt->gcExtraRootsTraceOp is called as the last thing is js_TraceRuntime with corresponding commnets. >@@ -502,16 +461,23 @@ JSBool XPCJSRuntime::GCCallback(JSContex ... >+ // We always want cx->globalObject to be unrooted. >+ JSContext *iter = nsnull, *acx; The comment is misleading. The intention is to do a custom marking in TraceXPConnectRoots so rephrase appropriately. Also move this flag setting into XPCJSRuntime::OnJSContextNew since it will be kept permanently AFAICS.
|
|
Assignee | |
Comment 57•14 years ago
|
||
> The comment is wrong as sizeof(JSString) == 4 words. Plus it is not
> compatible with fatvalues patch which would remove the doubles. Plus with
> fatvalues it would make sense to bump GC_CELL_SIZE to 4 words. Plus the
> XPConnect does not need colors for strings. So comment that currently only
> single extra color for object, function and xml things is supported and
> assert that in AssertValidColor.
I will remove the comment. If we decide to bump cell size, we can change the code accordingly. For now the assert is valid and meaningful.|
|
Assignee | |
Comment 58•14 years ago
|
||
> Comment at the function declaration that the color will only be applied to > object, function and xml things. Thats incorrect, especially functions need to be marked this way as well. > Move gcMarkColor into a sublass of JSTracer that the GC uses to avoid adding > to the runtime yet another flag that is relevant only during GC-marking. Also The GC stuff will move into its own struct from runtime. Right now it makes sense to keep everything in one place, which is runtime (which is wrong, but will change soon en-block). > the code should assert that no color-to-color transitions should happen, > i.e. either old or new color should be black. Why not? This seems like a bogus constraint. > Given that for now the only one color is supported rename this into > js_IsColoredGCThing(thing). We support 3 colors. Its just a name. Neither naming seems massively superior. > TraceJS currently is called before the conservative stack scanner scans the > stack, see js_TraceRuntime. So the latter should be changed so > rt->gcExtraRootsTraceOp is called as the last thing is js_TraceRuntime with > corresponding commnets. I don't think this makes sense. We want to mark strong roots first, and color them accordingly. I switch back to black before the conservative scanner runs and those are all strong (BLACK only roots). If we follow your approach and some XPConnect root is found by the stack scanner, we don't disclose parts of the GC heap to the cycle collector, and we might miss leaks. Please explain why this change is a good idea.
|
|
Assignee | |
Comment 59•14 years ago
|
||
Attachment #456399 -
Attachment is obsolete: true
Attachment #456499 -
Flags: review?(igor)
Attachment #456399 -
Flags: review?(igor)
|
|
||
Comment 60•14 years ago
|
||
(In reply to comment #58) > > Comment at the function declaration that the color will only be applied to > > object, function and xml things. > > Thats incorrect, especially functions need to be marked this way as well. I have wrote that "the color will only be applied to "object, *function* and xml things". > The GC stuff will move into its own struct from runtime. Right now it makes > sense to keep everything in one place, which is runtime (which is wrong, but > will change soon en-block). The suggestion is to move the flag into per GC-marking structure. This is not about moving GC-stuff from one per-runtime place to another. > > the code should assert that no color-to-color transitions should happen, > > i.e. either old or new color should be black. > > Why not? This seems like a bogus constraint. Right, that is not necessary, but see below. > > > Given that for now the only one color is supported rename this into > > js_IsColoredGCThing(thing). > > We support 3 colors. Its just a name. Neither naming seems massively superior. The observation is that the cycle collector needs only one extra state on top of marked/unmarked. IMO it suggests to have API supporting just that case. The API would query/set the extra state hiding the fact that this is implemented via spare bits in the GC bitmaps. > We want to mark strong roots first, and color > them accordingly. I switch back to black before the conservative scanner runs > and those are all strong (BLACK only roots). The conservative scan roots must have the same color as the rest of strong roots. This may not happen with the patch as it does: 1. marking as black autoroots and other runtime roots. 2. marking as gray things found during TraceXPConnectRoots(trc) unless they are already marked as black. 3. marking as black that the conservative scanner finds unless they already have black or gray color. So if a thing A is present on the native stack and among things that TraceXPConnectRoots finds, then A would have wrongfully gray color, not black.
|
|
Assignee | |
Comment 61•14 years ago
|
||
> > The GC stuff will move into its own struct from runtime. Right now it makes > > sense to keep everything in one place, which is runtime (which is wrong, but > > will change soon en-block). > > The suggestion is to move the flag into per GC-marking structure. This is not > about moving GC-stuff from one per-runtime place to another. For the record, you are making me change an API (compatibly) instead of hiding this in the engine. Whatever, I really want to land this patch and have no time to discuss window dressings. > The observation is that the cycle collector needs only one extra state on top > of marked/unmarked. IMO it suggests to have API supporting just that case. The > API would query/set the extra state hiding the fact that this is implemented > via spare bits in the GC bitmaps. For cross compartment GCs I may or may not need another of the bits, depending how on how I implement that. This is not an API. Its merely a friend function.
|
|
Assignee | |
Comment 62•14 years ago
|
||
Attachment #456499 -
Attachment is obsolete: true
Attachment #456575 -
Flags: review?(igor)
Attachment #456499 -
Flags: review?(igor)
|
|
Assignee | |
Comment 63•14 years ago
|
||
The color in JSTracer has to be initialized. I don't have BLACK defined in jsapi.h so I have to initialize to 0 here.
Attachment #456575 -
Attachment is obsolete: true
Attachment #456576 -
Flags: review?(igor)
Attachment #456575 -
Flags: review?(igor)
|
|
||
Comment 64•14 years ago
|
||
Comment on attachment 456576 [details] [diff] [review] patch >diff --git a/js/src/jsapi.h b/js/src/jsapi.h >--- a/js/src/jsapi.h >+++ b/js/src/jsapi.h >@@ -1123,16 +1123,17 @@ JS_MarkGCThing(JSContext *cx, void *thin > */ > #define JSVAL_IS_TRACEABLE(v) (JSVAL_IS_GCTHING(v) && !JSVAL_IS_NULL(v)) > #define JSVAL_TO_TRACEABLE(v) (JSVAL_TO_GCTHING(v)) > #define JSVAL_TRACE_KIND(v) (JSVAL_TAG(v) >> 1) > > struct JSTracer { > JSContext *context; > JSTraceCallback callback; >+ uint32 color; I should have been more clear here. My suggestion was top make a subclass of JSTracer that the GC would use and store the color there, it was not about expanding the tracer. In details, jsgc.h should have something like: struct GCMarker : JSTracer { GCMarker() : callback(NULL), color(0) {} uint32 color; }; This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *. >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp >--- a/js/src/jsgc.cpp >+++ b/js/src/jsgc.cpp >@@ -492,32 +492,54 @@ GetGCThingMarkBit(void *thing, size_t &b > { > CheckValidGCThingPtr(thing); > jsuword addr = reinterpret_cast<jsuword>(thing); > jsuword chunk = addr & ~GC_CHUNK_MASK; > bitIndex = (addr & GC_CHUNK_MASK) >> GC_CELL_SHIFT; > return reinterpret_cast<jsbitmap *>(chunk | GC_MARK_BITMAP_ARRAY_OFFSET); > } > >+static void >+AssertValidColor(void *thing, uint32 color) >+{ >+ JS_ASSERT_IF(color, color < JSGCArenaInfo::fromGCThing(thing)->list->thingSize / GC_CELL_SIZE); Check here also that the thing is object, function or xml here. > inline bool >-MarkIfUnmarkedGCThing(void *thing) >+MarkIfUnmarkedGCThing(void *thing, uint32 color) > { ... @@ -2311,74 +2356,54 @@ js_CallGCMarker(JSTracer *trc, void *thi > case JSTRACE_DOUBLE: { >- MarkIfUnmarkedGCThing(thing); >+ MarkIfUnmarkedGCThing(thing, BLACK); ... > case JSTRACE_STRING: ... >- if (!MarkIfUnmarkedGCThing(thing)) >+ if (!MarkIfUnmarkedGCThing(thing, BLACK)) > goto out; > if (!((JSString *) thing)->isDependent()) > goto out; > Repeated nit: default the color to BLACK to avoid changing the code marking strings and doubles. It will minimized the conflicts with the fatvalues patch. > JS_ASSERT(kind == GetFinalizableThingTraceKind(thing)); >- if (!MarkIfUnmarkedGCThing(thing)) >+ if (!MarkIfUnmarkedGCThing(thing, trc->color)) Repeated nit: remind in comments that trc->color does not applies to doubles and strings above so we default to BLACK color for them. >+JSBool >+nsXPConnect::IsGray(void *thing) >+{ >+ return js_GCThingIsMarked(thing, XPC_GC_COLOR_GRAY); >+} >+ > NS_IMETHODIMP > nsXPConnect::Traverse(void *p, nsCycleCollectionTraversalCallback &cb) > { > if(!mCycleCollectionContext) > return NS_ERROR_FAILURE; > > JSContext *cx = mCycleCollectionContext->GetJSContext(); > > uint32 traceKind = js_GetGCThingTraceKind(p); >+ Nit: this is unnecessary change. r+ with this addressed.
Attachment #456576 -
Flags: review?(igor)
|
|
Assignee | |
Comment 65•14 years ago
|
||
(In reply to comment #64) > (From update of attachment 456576 [details] [diff] [review]) > >diff --git a/js/src/jsapi.h b/js/src/jsapi.h > >--- a/js/src/jsapi.h > >+++ b/js/src/jsapi.h > >@@ -1123,16 +1123,17 @@ JS_MarkGCThing(JSContext *cx, void *thin > > */ > > #define JSVAL_IS_TRACEABLE(v) (JSVAL_IS_GCTHING(v) && !JSVAL_IS_NULL(v)) > > #define JSVAL_TO_TRACEABLE(v) (JSVAL_TO_GCTHING(v)) > > #define JSVAL_TRACE_KIND(v) (JSVAL_TAG(v) >> 1) > > > > struct JSTracer { > > JSContext *context; > > JSTraceCallback callback; > >+ uint32 color; > > I should have been more clear here. My suggestion was top make a subclass of > JSTracer that the GC would use and store the color there, it was not about > expanding the tracer. In details, jsgc.h should have something like: > > struct GCMarker : JSTracer { > GCMarker() : callback(NULL), color(0) {} > uint32 color; > }; > > This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *. SetColor takes a JSTracer, because thats what the API exposes to XPConnect, so in there I am going to upcast unsafely to a GCMarker? This is completely pointless and twice as ugly as before.
|
|
||
Comment 66•14 years ago
|
||
(In reply to comment #65) > > I should have been more clear here. My suggestion was top make a subclass of > > JSTracer that the GC would use and store the color there, it was not about > > expanding the tracer. In details, jsgc.h should have something like: > > > > struct GCMarker : JSTracer { > > GCMarker() : callback(NULL), color(0) {} > > uint32 color; > > }; > > > > This implies that the type for JSRuntime::gcMarkingTracer would be GCMarker *. > > SetColor takes a JSTracer, because thats what the API exposes to XPConnect, so > in there I am going to upcast unsafely to a GCMarker? You can safely upcast since at the moment of the call trc == trc->context->runtime->gcMarkingTracer. Asserting that and doing upcast is completely safe. In one of the patches in my queue I already have GCMarker to move some of the marking only state there.
|
|
||
Comment 67•14 years ago
|
||
(In reply to comment #66) > You can safely upcast since at the moment of the call > > trc == trc->context->runtime->gcMarkingTracer. > > Asserting that and doing upcast is completely safe. > > In one of the patches in my queue I already have GCMarker to move some of the > marking only state there. I was wrong here - the code in question that traverses xpconnect gray roots can be called with a non-gc tracer. But this points that SetColor should only be called when IS_GC_MARKING_TRACER is true. This also suggests to make SetColor a method of GCMarker
|
|
Assignee | |
Comment 68•14 years ago
|
||
Igor, I really have no time for another 10 iterations over this. I want to land this patch. I will move color back into the runtime, and we can cutify this in a separate patch.
|
|
||
Comment 69•14 years ago
|
||
(In reply to comment #68) > Igor, I really have no time for another 10 iterations over this. I want to land > this patch. OK, but you still needs IS_GC_MARKING_TRACER before SetColor calls no matter where the color will end up.
|
|
Assignee | |
Comment 70•14 years ago
|
||
Non-ADD_TO_CC values are being added to the CC graph after all. Debugging that at the moment and waiting for fatvals to land so I can land this patch.
|
|
Assignee | |
Comment 71•14 years ago
|
||
I went back to if (!ADD_TO_CC()) return NULL in ToParticipant as originally suggested by peterv. About to land this.
|
|
Assignee | |
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey
|
|
Assignee | |
Comment 73•14 years ago
|
||
peterv, you suggested to UNROOT global objects in one central place instead of every time is causing a ton of problems all over the place. context flags are not properly maintained and overwritten. I have fixed nsJSEnvironment but other places seem to exist. I will remove that part of the patch as well and go back to the original version that passed try server. For a patch that is this complex and hard to test, we should really reduce review nits to stuff that can't be done in follow-up patches.
|
|
||
Comment 74•14 years ago
|
||
That (comment 48, about JSOPTION_UNROOTED_GLOBAL) wasn't a review nit :-P. /be
|
|
Assignee | |
Comment 75•14 years ago
|
||
set mDefaultOptions instead of doing ::JS_GetOptions every time. This hopefully explains the test failures (looked like the "strict" flag got stuck)
Attachment #456576 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 76•14 years ago
|
||
Whatever it was, it wasn't strictly necessary for this patch I think. It was ugly before, we should have left it ugly here, and file a follow up to do it nicer.
|
|
||
Comment 78•14 years ago
|
||
(In reply to comment #73) > peterv, you suggested to UNROOT global objects in one central place instead of > every time I did no such thing, I suppose this was directed at igor.
|
|
Assignee | |
Comment 79•14 years ago
|
||
The patch seems to stick, except that we are leaking now. Sigh! I have seen the tree leak before the patch, too, but only on macosx. Now we leak everywhere. This patch is really fighting me all the way :) I will see if I have to back out again or diagnose in place.
Depends on: 579616
|
|
Assignee | |
Comment 80•14 years ago
|
||
re-landed: http://hg.mozilla.org/tracemonkey/rev/ef7f3e72ac08 this is leaking due to a pre-existing bug, mrbkap will fix
|
|
||
Comment 81•14 years ago
|
||
During normal browsing when zoomimg on google maps I got that assert: Assertion failure: JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL), at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:377 The backtrace is: #8 0x0027bce1 in JS_Assert (s=0x287fe7c "JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)", file=0x287fdc0 "/home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp", ln=377) at /home/igor/m/tm/js/src/jsutil.cpp:84 #9 0x01c2fdab in XPCJSRuntime::TraceXPConnectRoots (this=0xf5b3b0f0, trc=0xfff33cbc) at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:377 #10 0x01c2fc8f in XPCJSRuntime::TraceJS (trc=0xfff33cbc, data=0xf5b3b0f0) at /home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp:341 #11 0x0019cf49 in js_TraceRuntime (trc=0xfff33cbc) at /home/igor/m/tm/js/src/jsgc.cpp:2577 #12 0x0019d7b0 in GC (cx=0xf3231800) at /home/igor/m/tm/js/src/jsgc.cpp:3116 #13 0x0019e0a1 in GCUntilDone (cx=0xf3231800, gckind=GC_NORMAL) at /home/igor/m/tm/js/src/jsgc.cpp:3491 #14 0x0019e1f5 in js_GC (cx=0xf3231800, gckind=GC_NORMAL) at /home/igor/m/tm/js/src/jsgc.cpp:3545 #15 0x001397b5 in JS_GC (cx=0xf3231800) at /home/igor/m/tm/js/src/jsapi.cpp:2433 #16 0x01bfe17a in nsXPConnect::Collect (this=0xf670cd30) at /home/igor/m/tm/js/src/xpconnect/src/nsXPConnect.cpp:402 #17 0x01bfe1ca in nsXPConnect::GarbageCollect (this=0xf670cd30) at /home/igor/m/tm/js/src/xpconnect/src/nsXPConnect.cpp:410 #18 0x01752750 in nsJSContext::CC () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3613 #19 0x01752949 in nsJSContext::IntervalCC () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3702 #20 0x017528bb in nsJSContext::MaybeCC (aHigherProbability=1) at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3680 #21 0x017528f0 in nsJSContext::CCIfUserInactive () at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3690 #22 0x017529be in GCTimerFired (aTimer=0xe19deac0, aClosure=0x0) at /home/igor/m/tm/dom/base/nsJSEnvironment.cpp:3728 #23 0x0231cd60 in nsTimerImpl::Fire (this=0xe19deac0) at /home/igor/m/tm/xpcom/threads/nsTimerImpl.cpp:427 #24 0x0231cf97 in nsTimerEvent::Run (this=0xd2086560) at /home/igor/m/tm/xpcom/threads/nsTimerImpl.cpp:519 #25 0x023162c2 in nsThread::ProcessNextEvent (this=0xf754a470, mayWait=0, result=0xfff3404c) at /home/igor/m/tm/xpcom/threads/nsThread.cpp:547 #26 0x022a9989 in NS_ProcessNextEvent_P (thread=0xf754a470, mayWait=0) at nsThreadUtils.cpp:250 #27 0x021859ee in mozilla::ipc::MessagePump::Run (this=0xf755e160, aDelegate=0xf7521520) at /home/igor/m/tm/ipc/glue/MessagePump.cpp:118 #28 0x0237c17d in MessageLoop::RunInternal (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:219 #29 0x0237c0fd in MessageLoop::RunHandler (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:202 #30 0x0237c0a1 in MessageLoop::Run (this=0xf7521520) at /home/igor/m/tm/ipc/chromium/src/base/message_loop.cc:176 #31 0x0206e066 in nsBaseAppShell::Run (this=0xf41d20b0) at /home/igor/m/tm/widget/src/xpwidgets/nsBaseAppShell.cpp:175 #32 0x01df1d2d in nsAppStartup::Run (this=0xf3f4d0d0) at /home/igor/m/tm/toolkit/components/startup/src/nsAppStartup.cpp:192 #33 0x00f38683 in XRE_main (argc=2, argv=0xfff34744, aAppData=0xf750e380) at /home/igor/m/tm/toolkit/xre/nsAppRunner.cpp:3619 #34 0x08049847 in main (argc=2, argv=0xfff34744) at /home/igor/m/tm/browser/app/nsBrowserApp.cpp:158 (gdb) fr 8 #8 0x0027bce1 in JS_Assert (s=0x287fe7c "JS_HAS_OPTION(acx, JSOPTION_UNROOTED_GLOBAL)", file=0x287fdc0 "/home/igor/m/tm/js/src/xpconnect/src/xpcjsruntime.cpp", ln=377) at /home/igor/m/tm/js/src/jsutil.cpp:84
|
|
||
Comment 82•14 years ago
|
||
The above could come from http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpccomponents.cpp#3449.
|
||
Comment 83•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/47300d15cd1c
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•