Right now we have the ability to collect a single compartment at a time. This bug is about changing things so that we always collect entire compartment groups (see bug 751618). The benefit here is that we could put objects from different compartments into the same arena. This would allow us to reclaim some of the memory we lost with CPG. The challenge is how to handle obj->compartment(). For objects, we're thinking that shape arenas would still be compartment-specific. So we could replace this call with obj->shape->compartment(). For some specific callers, like checking cell->compartment()->needsBarrier(), we can move the data to the compartment group and use cell->compartmentGroup() instead, since arenas will be specific to a given compartment group. Hopefully that covers all uses.

Minor thing, but can you call these "compartment group" zones? This concept and the zones in bug 718121 are essentially the same thing, and using the same name makes merges between branches easier.

> This concept and the zones in bug 718121 are essentially the same thing Are they? A zone in bug 718121 is basically the closure of the "X can touch Y" relation -- that is, basically origin, right? We certainly could group compartments according to zones, and I doubt we'd want to be *less* granular than that, but might it be reasonable to group some or all the system compartments' memory together and leave content compartments as they are?

> A zone in bug 718121 is basically the closure of the "X can touch Y" relation -- that is, basically > origin, right? That's the *transitive* closure, of course. > might it be reasonable to group some or all the system compartments' memory together and leave > content compartments as they are? Although I am of course not a fan of CPG's memory regression, by not shoving all memory from an origin into one compartment, it makes per-tab memory reporting possible. That's a big win for us. If we used closure-over-can-touch as the grouping for allocation "zones" but keep per-compartment memory reporting (in some sane but not necessarily precise way; for example, the blame for fragmentation within a zone is shared between all the compartments, but we don't have to be perfect in how we assign the blame), and if we had reason to believe that closure-over-can-touch allocation zones are the right thing in most cases (*), I'd be OK with not having the flexibility to draw the allocation zone border wherever we please. (*) I'm not so sure it is. For example, the Facebook like button shouldn't be in the same compartment as my facebook.com window, because the lifetime of the like button isn't at all related to the lifetime of my Facebook window.

(In reply to Justin Lebar [:jlebar] from comment #2) > > This concept and the zones in bug 718121 are essentially the same thing > > Are they? A zone in bug 718121 is basically the closure of the "X can touch > Y" relation -- that is, basically origin, right? Not quite --- data in the same origin can be in different zones if there is no way they can observe each other (e.g. facebook widgets opened in independent tabs) and data in different origins can be in the same zone if they are associated with the same window or group of windows. A content zone in bug 718121 is all the data in a window and the other windows it has transitively opened and can potentially see effects in. There is a single additional zone for all chrome data. > We certainly could group compartments according to zones, and I doubt we'd > want to be *less* granular than that, but might it be reasonable to group > some or all the system compartments' memory together and leave content > compartments as they are? The zone concept in bug 718121 would effectively force our hand to group all system compartments together. Not sure if that is good or bad, but thinking some more I don't think I really understand the point of this bug. Fixing memory regression, sure, but won't all this be obviated by a compacting GC? (guess that's a ways off)

> The zone concept in bug 718121 would effectively force our hand to group all system compartments > together. That's probably fine, so long as we maintain the ability to report memory usage at the current level of granularity. Most system compartments have a similar lifetime. Indeed, it sounds like all the stuff in a zone likely has a similar lifetime, so it might make sense to group content that way too. I'm still not sure we should force ourselves into that position unless we have to... > I really understand the point of this bug. Fixing memory regression, sure, but won't all this be > obviated by a compacting GC? (guess that's a ways off) The situation would be improved by a compacting GC. But atm aiui each compartment stores 21 different kinds of things, and each kind of thing has to go in a separate arena. So that's potentially 4KB * 21 = 84KB of wasted space per compartment, or 16MB if you have 200 compartments. Of course we're not usually going to create an arena for each type of thing in each compartment, and we're not usually going to waste the entire arena. But that's at least one benefit that we can't get without a compacting GC. But fixing things Now is also nice. :)

Well, my intention here was that a group would consist of all the compartments from the same tab. The idea is that compartments in the same group should, roughly, die at the same time. And they shouldn't have pointers to any other groups besides a chrome group. That's what we need for GC. I don't know if zones will have these properties or not, but I hope they do. If not, we'll need some other notion. As Justin said, this is a memory savings even with compacting GC. It's likely we're going to stick with the existing arena model for the tenured generation for a while, so we need something like this.

This could be handy for the cycle collector, too, with something like bug 754495.

(In reply to Bill McCloskey (:billm) from comment #6) > Well, my intention here was that a group would consist of all the > compartments from the same tab. The idea is that compartments in the same > group should, roughly, die at the same time. And they shouldn't have > pointers to any other groups besides a chrome group. That's what we need for > GC. I don't know if zones will have these properties or not, but I hope they > do. If not, we'll need some other notion. Yeah, this is the same as the description in comment 4, except that zones can include data from multiple tabs when they open each other with window.open etc. (in which case they will be able to point to and observe effects in those other tabs).

> Well, my intention here was that a group would consist of all the compartments from the same tab. > [...] They shouldn't have pointers to any other groups besides a chrome group. The second condition is what I understand bhackett's zones gets us. But that's not "all compartments from the same tab"; like he says, you can have pointers to any window you transitively opened.

We removed the MemShrink tag because it seems more appropriate to track it with bug 764220.

I've been having trouble measuring how this affects memory usage. Nick offered to do some measurements. In the meantime, I'll work on fixing a few remaining bugs. I think it should be stable to test with though.

Comment on attachment 713232 [details] [diff] [review] rolled up patch on top of 8987eff12bd8 Review of attachment 713232 [details] [diff] [review]: ----------------------------------------------------------------- I've just looked at the memory reporting so far. In a (simplified) example like this: - top(http://www.valgrind.org/, id=14) - active/window(http://www.valgrind.org/) - layout - js/compartment(http://www.valgrind.org/) - dom - style-sheets - js/zone(0x7f1c6ff1f800) I was expecting the 4th line to instead be this: - js/zone(0x7f1c6ff1f800)/compartment(http://www.valgrind.org/) but I guess you omitted that because it can be inferred from the 7th line? ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2158,5 @@ > + if (nsCOMPtr<nsPIDOMWindow> piwindow = do_QueryInterface(native)) { > + // The global is a |window| object. Use the path prefix that > + // we should have already created for it. > + if (mTopWindowPaths->Get(piwindow->WindowID(), &path)) > + path.AppendLiteral("/js/"); Nit: this can be "/js-". That will give a path like "top(foo)/js-zone/..." instead of "top(foo)/js/zone/...". It might not seem much different, but it makes it clear that there will only ever be one zone per top window.

So, the following things are now stored at a zone level rather than a compartment level: - strings (normal and short) and their chars - type objects and their data - IonCodes - XML objects :) I guess strings are done that way to allow cross-compartment (intra-zone) string sharing. What about type objects and IonCodes?

Initial measurements are very promising! There's a lot of noise in the browser, but I was able to get reasonably steady results like this. 1. Create a session containing just about:memory?verbose. 2. Restart, hit "restore previous session". 3. Repeat step 2 a few times; the first few are noisy but then it settles down, with the main numbers (explicit, resident) varying by less than 1 MiB. Below are some sample numbers from starting up a 64-bit build. They all look really good, exactly like what I was hoping for! Basically, "unused-gc-things" dropped by ~4.8 MiB (to only 0.2 MiB), and "explicit" and "resident" both fell by ~4.6 MiB. EXPLICIT: shrunk by 7.1% (before) 65,572,029 B (100.0%) -- explicit ├──31,338,496 B (47.79%) -- js-non-window │ ├──26,866,824 B (40.97%) -- compartments (after) 60,935,117 B (100.0%) -- explicit ├──26,489,864 B (43.47%) -- js-non-window │ ├──22,120,592 B (36.30%) -- zones RESIDENT: shrunk by 4.6% (before) 101,756,928 B ── resident (after) 97,095,680 B ── resident TINY COMPARTMENTS: shrunk by a lot (before) │ │ │ ├──────42,840 B (00.07%) -- compartment([System Principal], chrome://global/content/bindings/toolbarbutton.xml) │ │ │ │ ├──32,768 B (00.05%) -- gc-heap │ │ │ │ │ ├──23,680 B (00.04%) ── unused-gc-things │ │ │ │ │ └───9,088 B (00.01%) ── sundries │ │ │ │ └──10,072 B (00.02%) ── other-sundries │ │ │ ├──────33,760 B (00.05%) -- compartment([System Principal], chrome://browser/content/places/placesOverlay.xul) │ │ │ │ ├──24,576 B (00.04%) -- gc-heap │ │ │ │ │ ├──23,200 B (00.04%) ── unused-gc-things │ │ │ │ │ └───1,376 B (00.00%) ── sundries │ │ │ │ └───9,184 B (00.01%) ── other-sundries │ │ │ ├──────29,376 B (00.04%) -- compartment([System Principal], chrome://global/content/bindings/scrollbar.xml) │ │ │ │ ├──20,480 B (00.03%) -- gc-heap │ │ │ │ │ ├──19,432 B (00.03%) ── unused-gc-things │ │ │ │ │ └───1,048 B (00.00%) ── sundries │ │ │ │ └───8,896 B (00.01%) ── other-sundries (after) │ │ │ ├──────17,328 B (00.03%) -- compartment([System Principal], chrome://global/content/bindings/toolbarbutton.xml) │ │ │ │ ├───9,048 B (00.01%) ── other-sundries │ │ │ │ └───8,280 B (00.01%) ── gc-heap/sundries │ │ │ ├───────9,072 B (00.01%) -- compartment([System Principal], chrome://browser/content/places/placesOverlay.xul) │ │ │ │ ├──8,160 B (00.01%) ── other-sundries │ │ │ │ └────912 B (00.00%) ── gc-heap/sundries │ │ │ ├───────8,496 B (00.01%) -- compartment([System Principal], chrome://global/content/bindings/scrollbar.xml) │ │ │ │ ├──7,872 B (00.01%) ── other-sundries │ │ │ │ └────624 B (00.00%) ── gc-heap/sundries OVERALL GC HEAP: shrunk by 22.2% ("unused/gc-things" shrunk by -95.5%!) (before) 18,874,368 B (100.0%) -- js-main-runtime-gc-heap-committed ├──11,700,840 B (61.99%) -- used │ ├──11,183,248 B (59.25%) ── gc-things │ ├─────278,528 B (01.48%) ── chunk-admin │ └─────239,064 B (01.27%) ── arena-admin └───7,173,528 B (38.01%) -- unused ├──5,027,224 B (26.64%) ── gc-things ├──1,097,728 B (05.82%) ── arenas └──1,048,576 B (05.56%) ── chunks (after) 14,680,064 B (100.0%) -- js-main-runtime-gc-heap-committed ├──12,407,320 B (84.52%) -- used │ ├──12,015,656 B (81.85%) ── gc-things │ ├─────212,992 B (01.45%) ── chunk-admin │ └─────178,672 B (01.22%) ── arena-admin └───2,272,744 B (15.48%) -- unused ├──1,048,576 B (07.14%) ── chunks ├────995,328 B (06.78%) ── arenas └────228,840 B (01.56%) ── gc-things More measurements coming soon.

We can't run the full JS memory reporter for telemetry, but is there some data we /could/ collect for telemetry which would help us validate the changes here (and help us not regress them in the future)?

> We can't run the full JS memory reporter for telemetry, but is there some > data we /could/ collect for telemetry which would help us validate the > changes here (and help us not regress them in the future)? First, let's imagine we can run the full JS memory reporter. I guess "js-main-runtime-gc-heap-committed/unused/gc-things" is the most likely candidate. The ratio between "js-main-runtime-gc-heap-committed/used" and "js-main-runtime-gc-heap-committed/unused" is another good candidate. But both of those require touching all the live heap elements, and so are no good for telemetry. The only other thing I can think of is "js-gc-heap", which we already track for telemetry. But it's a rather oblique measure of the impact of zones.

> But both of those require touching all the live heap elements, and so are no good for telemetry. We couldn't keep a counter somewhere which we decrement every time we create an object and increment every time we allocate a new chunk or gc an object? I'm not convinced that's worth while, though; maybe js-gc-heap is sufficient for our purposes.

I did an before/after run on AWSY. Results are at https://areweslimyet.com/?series=njn-zones. (And instructions on how to do your own custom AWSY run are at Here are the explicit results. (I've attached a screenshot of the explicit graph.) EXPLICIT light blue: Fresh start 56.25MiB Δ -6.90MiB light green:Fresh start [+30s] 51.97MiB Δ -7.25MiB pink: After TP5 313.14MiB Δ -4.87MiB orange: After TP5 [+30s] 304.33MiB Δ -6.63MiB dark green: After TP5 [+30s, forced GC] 296.90MiB Δ -6.00MiB dark blue: After TP5, tabs closed 261.16MiB Δ -8.46MiB red: After TP5, tabs closed [+30s] 138.92MiB Δ 34.27MiB purple: After TP5, tabs closed [+30s, forced GC] 124.92MiB Δ 23.03MiB Excluding the red and purple lines, they were all good: 4.8 to 8.5 MiB better. But the red and purple lines (which measure after all the tabs have been closed) were much worse! Why? Consider the red line first. Here's a partial breakdown. (before) - 104.65 MiB explicit - 47.42 MiB (45.32%) js-non-window - 30.95 MiB (65.27%) compartments - 13.39 MiB (28.23%) gc-heap - 12.82 MiB (95.80%) decommitted-arenas - 576.00KiB (4.20%) chunk-admin - 0B (0%) unused-arenas - 0B (0%) unused-chunks - 3.08 MiB (6.50%) runtime - 21.99 MiB (21.02%) heap-unclassified - 6.60 MiB (6.30%) window-objects (after) - 138.92 MiB explicit - 77.69 MiB (55.92%) js-non-window - 48.07 MiB (61.88%) gc-heap - 47.01 MiB (97.79%) decommitted-arenas - 1,088.00KiB (2.21%) chunk-admin - 0B (0%) unused-arenas - 0B (0%) unused-chunks - 26.48 MiB (34.08%) zones - 3.14 MiB (4.04%) runtime - 23.46 MiB (16.89%) heap-unclassified - 8.45 MiB (6.09%) window-objects In summary: - window-objects: +1.8 MiB - heap-unclassified: +1.5 MiB - JS: +30 MiB The "window-objects" is because there was an extra "1,367.42 KiB (15.80%) top none" window in the "after" case. I checked, that appears in some runs and not in others, so it's ignorable noise. No idea about the "heap-unclassified", but it could well be noise similar to window-objects. The main difference is JS, and it's almost entirely due to "decommitted-arenas" being much higher -- 38.2 MiB higher, which is greater than the 34.3 MiB explicit increase. ("chunk-admin" was ~2x higher as well, which corroborates the "more chunks are present" story.) I looked at the purple numbers closely, the story is much the same there. Here are the resident numbers. RESIDENT light blue: Fresh start 103.68MiB Δ -4.70MiB light green:Fresh start [+30s] 100.05MiB Δ -6.57MiB pink: After TP5 369.96MiB Δ 3.23MiB orange: After TP5 [+30s] 361.68MiB Δ 1,352.00KiB dark green: After TP5 [+30s, forced GC] 356.42MiB Δ 3.37MiB dark blue: After TP5, tabs closed 324.45MiB Δ -1,512.00KiB red: After TP5, tabs closed [+30s] 208.71MiB Δ 112.00KiB purple: After TP5, tabs closed [+30s, forced GC] 198.95MiB Δ -4.17MiB Resident is a noisier measurement in general, and these measurements reflect that. The important thing is that the red and purple lines didn't get the big regression. In some sense, the increase in decommitted-arenas doesn't matter that much... except on 32-bit platforms (including Windows!) where virtual address space is moderately tight. But it would definitely be nice to (a) understand it, and (b) avoid it if possible. billm said on IRC that the details of the user/system chunk split has changed a bit with the patch, and that he'll poke around with that aspect of the patch to see if it helps. (We saw big wins when that split was first added in Firefox 7; we also didn't have decommitting at that time.)

> (And instructions on how to do your own custom AWSY run are at ... https://areweslimyet.com/faq.htm#how-can-i-request-additional-awsy-tests-on-specific-changesets-or-try-pushes (Sorry, that got chopped off somehow.)

Comment on attachment 713232 [details] [diff] [review] rolled up patch on top of 8987eff12bd8 f=me. Looking good, just need to work out the "decommitted-arenas" usage.

The decommitted arenas bug is fixed, and everything looks green on try. I'm going to start putting patches up for review. For the most part, I've tried to ensure that intermediate patches compile and everything on their own. Here's a basic overview of how the patches are structured: 1. Right now Zone is typedefed to JSCompartment. The first patch makes it a separate type. All the GC-related fields go in the zone, as well as a little bookkeeping. Zones and compartments are still 1:1; a Zone is just a field of JSCompartment. Iterators like GCZoneGroupIter also get their own separate definitions. 2. Whereas before we could use CellIter to iterate over all the cells in a compartment, now we instead need to iterate over the zone. In some cases this is fine, but in other cases we're only interested in a particular compartment and we need to skip over cells from other compartments in the zone. 3. The above scheme is pretty inefficient if stuff like the following happens: ...during GC... iterate over all compartments C being collected: for each cell in the C->zone(): if cell->compartment() != C: continue // else do work If there's a zone with many compartments (like the system compartment) then this will iterate over the same cell many times. It's much better to structure the code this way: iterate over all zones Z: for each cell in Z: // do work However, this requires some restructuring in the methodjit, IonMonkey, and TI. The next few patches do that. 4. Then we need to fix up the memory reporters to handle zones. 5. There are patches to add a compartment() method to JSObject, JSScript, Shape, and BaseShape. I've added a compartment field to JSScript and BaseShape, so those types use that. JSObject and Shape get their compartment by going through their BaseShape. 6. The cycle collector needs to be modified to do zone merging rather than compartment merging. 7. With 5 and 6 done, the Cell::compartment() method can be eliminated. 8. Now we can permit multiple compartments in a zone. Each zone holds a vector of compartments within it. All the iterators need to be changed to permit all the different kinds of iteration (all compartments, all zones, all compartments in a zone, etc.). The JS_NewGlobalObject allows you to specify the zone in which the new compartment should belong. 9. Finally, nsGlobalWindow.cpp needs to be changed to make roughly one zone per tab.

This makes a separate Zone type as well as separate iterators. I've added the function ZONE_TO_COMPARTMENT to convert a zone to a compartment (since they're still 1:1). This function will gradually be eliminated in later patches. JSCompartment has a zone as a field.

Straightforward. No code changes.

This patch switches CellIter to iterate over a Zone instead of a JSCompartment. In a bunch of places I had to test whether the resulting cell is from the compartment we're interested in. I suspect this is pretty inefficient for discardJitCode and JSCompartment::sweep (as well as the TI stuff called by sweep). That stuff will be fixed in later patches.

This changes some methodjit code to operate on zones instead of compartments. The goal is to solve some of the perf problems with the CellIter patch.

This patch does a similar thing for IonMonkey. It also moves discardJitCode to be a method of Zone rather than compartment, which solves one of the iteration issues mentioned above. All the IonContext and AutoFlusher changes were necessary because the stack of AutoFlushers was stored on the compartment. However, I think it can be just as easily stored with the runtime.

I think this file used to be part of xpconnect, and the style was really bothering me. The only real change is that I removed a JS_THREADSAFE #ifdef around the entire file. It didn't seem to serve a purpose, and it covered up a lot of compile errors in my non-threadsafe build.

This does kinda what you would expect. Objects, shapes, and scripts are reported per-compartment. Types, strings, and IonCode are per-zone because we don't have any way of finding their compartment. (And in fact strings can be shared between compartments in a zone.) The FIXME thing is fixed later on in the patch stack when we have an actual separation between compartments and zones. I added a void* pointer in JSCompartment that can point to the CompartmentStats structure during memory reporting. This is kinda hacky, but I couldn't think of any better way to efficiently get that data.

PConnect is now JS style (sortof) so I tried to bring this stuff in line. Mostly I just wanted to kill all the extra whitespace.

I just kept iterating on this until the results seemed reasonable. I'd appreciate some feedback.

This patch moves the typeLifoAlloc to the zone. As a consequence, I had to make nukeTypes apply to an entire zone. The reason for doing this was to fix the CellIter issue in JSCompartment::sweep. Also, typeLifoAlloc is probably something that makes sense to put in the zone to save memory for small compartments.

Very simple.

This one also goes through the BaseShape.

I'm undecided whether to do it this way or to use cx->compartment->zone. At the time, this seemed better, but I'll try to benchmark this once I finish posting the patches.

This changes the cycle collector so that we merge zones rather than compartments. I had to do this because it's no longer possible to get the compartment of an arbitrary GC thing, which is needed for merging.

With these patches, strings no longer belong to a compartment--they just belong to a zone. I had to update a few uses outside the engine.

Again, strings no longer belong to a specific compartment, so we can't try to enter a string's compartment. I fixed this by no longer requiring JS_GetStringCharsZAndLength to be called from the string's compartment (or zone, which is what the assertion would really check). It looks like we don't allocate from the path, so I don't think it should matter.

This takes out the accessor and fixes a few miscellaneous places where it was used.

This adds a vector of compartments to the zone. A bunch of iterators have to be changed so that they iterate over the right entities now. I also changed JS_NewGlobalObject so you can decide what zone to put the global in. You can ask for a fresh zone, for an existing zone, or for the special "system" zone (which is used by the browser but not the JS engine). The atoms compartment gets its own special zone.

This uses the window's GetTop method to decide how to group globals into zones. It seems to roughly give us a per-tab grouping, although there are a few extra zones in there that don't correspond to normal windows. However, I think this works well enough.

This patch allows us to destroy compartments from zones before the entire zone is destroyed. I noticed that some pages create a new compartment every few seconds, only for it to die a little later. A compartment will get destroyed whenever it doesn't have any more base shapes or scripts. Because of how we mark, this should mean that a compartment with live objects or shapes will never get destroyed, since these things cause a base shape to be marked. One subtle point is that a compartment that is added after an incremental GC starts is automatically considered marked.

Comment on attachment 714618 [details] [diff] [review] 3. CellIter changes Review of attachment 714618 [details] [diff] [review]: ----------------------------------------------------------------- What is the future of Cell::compartment() vis a vis generational GC? It seems like many of the changes in this patch are correcting for the case where CellIter returns a cell not in the queried compartment. This seems a bit of a footgun and it'd be nice to avoid by having (optionally) CellIter or a similar class filter out things not from the original compartment. ::: js/src/jsinfer.cpp @@ +2949,1 @@ > unsigned count = object->getPropertyCount(); This should filter out objects not from the target compartment. @@ +2963,2 @@ > RootedScript script(cx, i.get<JSScript>()); > if (script->types) { Ditto. @@ +3103,2 @@ > RootedScript script(cx, i.get<JSScript>()); > if (script->hasAnalysis() && script->analysis()->ranInference()) Ditto. @@ +3110,1 @@ > TypeObject *object = i.get<TypeObject>(); Ditto.

(In reply to Brian Hackett (:bhackett) from comment #43) > Comment on attachment 714618 [details] [diff] [review] > 3. CellIter changes > > Review of attachment 714618 [details] [diff] [review]: > ----------------------------------------------------------------- Hmm, I should have stopped to think a minute before reviewing this. Ignore these comments, the patch is fine.

Fantastic stuff, billm! > The decommitted arenas bug is fixed What was the problem? Can you attach a new rolled-up patch so I can do some more measurements on Monday? Thanks.

Comment on attachment 714645 [details] [diff] [review] 16. Changes outside the JS engine r=me

Comment on attachment 714636 [details] [diff] [review] 10. TI changes Review of attachment 714636 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Zone.cpp @@ +142,5 @@ > +Zone::sweep(FreeOp *fop, bool releaseTypes) > +{ > + /* > + * Periodically release observed types for all scripts. This is safe to > + * do when there are no frames for the compartment on the stack. s/compartment/zone/

Comment on attachment 714643 [details] [diff] [review] 15. Update cycle collector merging Review of attachment 714643 [details] [diff] [review]: ----------------------------------------------------------------- Is compartment nuking probably going to wipe out the entire tab zone when we close the tab? This is suboptimal, in that it will add intra-zone cross-compartment edges as self-edges on the zone's node in the CC graph, but it should still generate a strictly smaller graph than we currently do, so no big deal. I filed bug 842137 for that, in case I feel like looking into it at some point in the future. I'm not sure there's any way other than adding an explicit check for if the child is in the same zone, so maybe it isn't worthwhile. ::: js/src/jsfriendapi.cpp @@ +325,5 @@ > return comp->zone(); > } > > JS_FRIEND_API(bool) > +js::IsSystemCompartment(JSCompartment *comp) OOC, why did you drop the const here? ::: js/xpconnect/src/nsXPConnect.cpp @@ +2449,4 @@ > * or is a cross-compartment wrapper. In the former case, we don't need to > * represent these edges in the CC graph because JS objects are not ref counted. > * In the latter case, the JS engine keeps a map of these wrappers, which we > * iterate over. Please add a sentence saying that we will end up adding intrazone cross-compartment edges to the CC graph, even though we don't really need them.

Comment on attachment 714627 [details] [diff] [review] 6. Memory reporting style changes Review of attachment 714627 [details] [diff] [review]: ----------------------------------------------------------------- I have a vague memory of the JS_THREADSAFE define being there for a reason, but I can't recall it now. If it doesn't cause problems it seems like removing it is an improvement.

Comment on attachment 714628 [details] [diff] [review] 7. Main JS-engine memory reporting changes Review of attachment 714628 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Nice work. ::: js/public/MemoryMetrics.h @@ +142,5 @@ > + gcHeapTypeObjects(0), > + gcHeapIonCodes(0), > +#if JS_HAS_XML_SUPPORT > + gcHeapXML(0), > +#endif Don't forget to remove all the XML stuff when you rebase. @@ +188,5 @@ > + hugeStrings.append(other.hugeStrings); > + } > + > + // These fields can be used by embedders. > + void *extra1; "This field". @@ +207,5 @@ > + size_t typeObjects; > + > + js::Vector<HugeStringInfo, 0, js::SystemAllocPolicy> hugeStrings; > + > + // The size of all the live things in the GC heap. This function just measures GC things that belong to the zone but not to a compartment within the zone, right? Please expand the comment to explain this. ::: js/src/jscompartment.h @@ +376,5 @@ > } > #endif > + > + /* Used by memory reporters. */ > + void *extraData; This is not a good name. Maybe |compartmentStats|, and expand the comment to explain that it's only valid during memory reporting. Come to think of it, this will be a dangling pointer once memory reporting ends. Can you NULL this field out in all compartments at the end of reporting? And please initialize it to NULL in JSCompartment's constructor. ::: js/src/jsmemorymetrics.cpp @@ +323,1 @@ > } You had to remove the assertion in CompartmentStats::gcHeapThingsSize(). I think you can replicate it here, more or less. If you add rtStats->gcHeapGcThings to the sum of all zStats->gcHeapArenaAdmin values and the sum of all zStats->gcHeapUnusedGcThings values, it should be a multiple of the arena size. I hope you can. Getting all these numbers to add up correctly isn't easy, and we've had bugs along these lines before. So assertions help.

Comment on attachment 714629 [details] [diff] [review] 8. Style fixes for XPC mem reporting Review of attachment 714629 [details] [diff] [review]: ----------------------------------------------------------------- Removing the whitespace is good. As for the other changes... I think the whole idea of taking a file written in one style and saying "we're using a different style now" (as was done a while back; I'm not blaming billm for this) is pretty bogus if you *don't actually change the style when you make the decision*. Because you end up with a complete mish-mash and some people know that the style actually used in the file shouldn't be copied... sigh. Anyway, I won't hold up progress in this bug arguing over such minutiae: r=me.

Comment on attachment 714634 [details] [diff] [review] 9. XPC memory reporting Review of attachment 714634 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +1418,5 @@ > + > + for (size_t i = 0; i != zoneStatsVector.length(); i++) { > + free(zoneStatsVector[i].extra1); > + // No need to free |extra2| because it's a static string. > + } Put zone stuff before compartment stuff. @@ +1435,5 @@ > + // This is the |cJSPathPrefix|. Each worker has exactly two compartments: > + // one for atoms, and one for everything else. > + nsAutoCString cJSPathPrefix(mRtPath); > + cJSPathPrefix += NS_LITERAL_CSTRING("zone(web-worker)/"); > + aZoneStats->extra1 = strdup(cJSPathPrefix.get()); Too much cargo-culting here :) - Rename cJSPathPrefix as pathPrefix. - The comment is wrong. Each worker has exactly one zone. - You're building a literal CString from a literal char* just so you can immediately pull out the literal char*. Just treat this one like aCompartmentStats->extra2, and then you won't need to free it in ~WorkerJSRuntimeStats. ::: js/public/MemoryMetrics.h @@ +183,5 @@ > ADD(gcHeapXML); > #endif > > ADD(stringCharsNonHuge); > + ADD(typeObjects); Oh, I guess this should have really been in patch 7? ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +1556,5 @@ > namespace xpc { > > static nsresult > +ReportZoneStats(const JS::ZoneStats &zStats, > + const nsACString &path, Can you rename |path| as |pathPrefix| for consistency with ReportCompartmentStats? @@ +1563,4 @@ > { > size_t gcTotal = 0, gcHeapSundries = 0, otherSundries = 0; > > + CREPORT_GC_BYTES(path + NS_LITERAL_CSTRING("gc-heap/arena-admin"), Hmm, the CREPORT_* macros are named that because they are for compartments. (We also have RREPORT_* macros for the runtime.) But now they're used for zones too. Maybe rename as ZCREPORT_*? You'll need to update the comments above SUNDRIES_THRESHOLD, CREPORT_BYTES, and CREPORT_BYTES2. @@ +1606,5 @@ > + CREPORT_GC_BYTES(path + NS_LITERAL_CSTRING("gc-heap/xml"), > + zStats.gcHeapXML, > + "Memory on the garbage-collected JavaScript " > + "heap that holds E4X XML objects."); > +#endif Again: please remove all the XML stuff before landing. @@ +1609,5 @@ > + "heap that holds E4X XML objects."); > +#endif > + > + CREPORT_BYTES(path + NS_LITERAL_CSTRING("type-inference/type-objects"), > + zStats.typeObjects, This is the only measurement under "type-inference/" at the zone level, right? In which case, please rename it just "type-objects", which makes it clear that there aren't other zone-level measurements relating to type inference. @@ +1655,5 @@ > + REPORT_GC_BYTES(path + NS_LITERAL_CSTRING("gc-heap/sundries"), > + gcHeapSundries, > + "The sum of all the gc-heap " > + "measurements that are too small to be worth showing " > + "individually."); I realize it was pre-existing... this string could be made to fit on two lines. @@ +1664,5 @@ > + REPORT_BYTES(path + NS_LITERAL_CSTRING("other-sundries"), > + nsIMemoryReporter::KIND_HEAP, otherSundries, > + "The sum of all the non-gc-heap " > + "measurements that are too small to be worth showing " > + "individually."); Ditto. @@ +1909,5 @@ > + nsCString path(static_cast<char *>(zStats.extra1)); > + > + rv = ReportZoneStats(zStats, path, cb, closure, &gcTotal); > + NS_ENSURE_SUCCESS(rv, rv); > + } Put zone stuff before compartment stuff. @@ +2152,5 @@ > free(compartmentStatsVector[i].extra2); > } > + > + for (size_t i = 0; i != zoneStatsVector.length(); ++i) > + free(zoneStatsVector[i].extra1); Put zone stuff before compartment stuff. @@ +2160,5 @@ > + // Get the compartment's global. > + nsXPConnect *xpc = nsXPConnect::GetXPConnect(); > + JSContext *cx = xpc->GetSafeJSContext(); > + JSCompartment *comp = js::GetAnyCompartmentInZone(zone); > + nsCString path; Rename as |pathPrefix|, to match initExtraCompartmentStats(). @@ +2177,5 @@ > + } else { > + path.AssignLiteral("explicit/js-non-window/zones/"); > + } > + } else { > + path.AssignLiteral("explicit/js-non-window/zones/"); There are only really two cases here: (a) we have a top window (b) we don't have a top window. Currently (b) is covered by three |else| branches. I think you should be able to combine the conditions somehow so you only need to write |path.AssignLiteral("explicit/js-non-window/zones/");| once. @@ +2182,5 @@ > + } > + > + char zoneName[32]; > + sprintf(zoneName, "%p", (void *)zone); > + nsCString zName(zoneName); Use nsPrintfCString for this. There are examples in this file that you can crib from. @@ +2229,5 @@ > > + if (needZone) { > + char zoneName[32]; > + sprintf(zoneName, "%p", (void *)js::GetCompartmentZone(c)); > + nsCString zName(zoneName); Use nsPrintfCString for this.

Looking at the memory reporting more closely, there are three minor changes I'd like you to make to the paths. NON-WINDOW OBJECTS ------------------ Currently it's like this. -- js-non-window -- zones -- zone(0x7f4ee0b5a800) ++ compartment([System Principal], ... ++ compartment([System Principal], ... ++ compartment([System Principal], ... ++ compartment([System Principal], ... ++ gc-heap ── other-sundries ++ zone(...) ++ runtime ++ gc-heap Here, a zone's size is measured as all the zone-specific data *plus* the sum of all the compartment sizes. Doing it this way makes it clear which compartments belong to which zone, which is good. This doesn't need changing. WORKERS ------- Currently it's like this. -- workers/workers()/worker(resource://gre/modules/... ++ compartment(web-worker) ++ gc-heap ++ runtime ++ zone(web-worker) ── compartment(web-worker-atoms)/other-sundries Here, a zone's size is measured as just the zone-specific data; the compartment sizes aren't put under the zone. In one sense they don't need to be, because (unlike the "js-non-window" case) there's only one zone, so we know which zone the two compartments belong to. But to be consistent with "js-non-windows" the compartments would be reported under the zone, like this: -- workers/workers()/worker(resource://gre/modules/... -- zone(web-worker) ++ compartment(web-worker) ── compartment(web-worker-atoms)/other-sundries ++ gc-heap ++ ... ++ gc-heap ++ runtime I think that's easy to do and worth if for the consistency. Change #1: please do this... and use the "zone(<address>)" rather than "zone(web-worker)" for consistency with the "js-non-window" case. WINDOWS ------- Currently it's like this. -- window-objects -- top(chrome://browser/content/browser.xul, id=3) -- active -- window(chrome://browser/content/browse... ++ js/compartment([System Principal], ... ++ layout ++ dom ── style-sheets ── property-tables -- window(about:blank) ++ js/compartment([System Principal], ... ++ dom ── style-sheets ++ js/zone(0x7f4ed5d80000) There are two ways to go here. Either we can tie each compartment's memory usage to its window, and report the zone in pieces (like above). Or we can report each zone in a single place (consistent with the non-window cases above) but lose the clear connection between compartments and windows, like this: -- window-objects -- top(chrome://browser/content/browser.xul, id=3) ++ js-zone(0x7f4ed5d80000)/ ++ compartment([System Principal], ... ++ compartment([System Principal], ... -- active -- window(chrome://browser/content/browse... ++ layout ++ dom ── style-sheets ── property-tables -- window(about:blank) ++ dom ── style-sheets ++ js/zone(0x7f4ed5d80000) The window-centric easily answers the question "how much memory would we save if this window object disappeared". The zone-centric view easily answers the question "how much memory is used by JS for this top window?" I think window-centric is probably better. Summing the JS values to get the zone's size is tedious but simple, whereas sometimes manually working out which compartment belongs to which window isn't easy. Also, sometimes windows with different names have compartments with the same name (this is common for "about:blank" compartments) and the zone-centric view would collapse those same-named compartments together. So let's keep the window-centric view, but the "js/zone" can become "js-zone", because there's always only one zone per top window (as I mentioned in comment 12). And I just realized that "js/compartment" can become "js-compartment" because there's always one compartment per window. Change #2 and #3: please make these two changes (in initExtra{Zone,Compartment}Stats). Thanks!

Finally, I'd love to do some more pre-landing measurements. Can you attach a rolled-up patch and tell me what version it applies against? Thanks.

Comment on attachment 714650 [details] [diff] [review] 19. Allow many compartments in one zone In js/src/jsgc.cpp, js::NewCompartment: + ScopedJSDeletePtr<Zone> zoneHolder; + if (!zone) { + zone = cx->new_<Zone>(rt); + if (!zone) return NULL; + if (!zone->init(cx)) + return NULL; + + zoneHolder.reset(zone); + zone->setGCLastBytes(8192, GC_NORMAL); + } Even though Zone::init() is infallible at the moment, shouldn't the call to zoneHolder.reset() go before it?

Comment on attachment 714656 [details] [diff] [review] 21. Compartment sweeping Review of attachment 714656 [details] [diff] [review]: ----------------------------------------------------------------- In SweepCompartments, can you add a comment to explain what the connection is between keepAtleastOne, foundOne and the condition "!(read == end && !foundOne)"? I found this pretty confusing.

Comment on attachment 714654 [details] [diff] [review] 20. Group compartments into zones by tab Review of attachment 714654 [details] [diff] [review]: ----------------------------------------------------------------- r- while we get the API and the sandbox stuff sorted out. ::: dom/base/nsGlobalWindow.cpp @@ +1960,5 @@ > + top = aNewInner->GetTop(); > + } > + JSObject *sameZone = NULL; > + if (top) { > + sameZone = top->GetGlobalJSObject(); So, |top| here will be an outer window, which means that GetGlobalJSObject() will return the outer window proxy. This object isn't a global object in the JS sense, though it should be in the compartment of the current global. Is that enough? @@ +1961,5 @@ > + } > + JSObject *sameZone = NULL; > + if (top) { > + sameZone = top->GetGlobalJSObject(); > + } I talked to smaug, and I think this should work well, because there aren't any docshells on the other side of the chrome->content boundary with content lifetime. The parent of the top-level content window is a chrome window with a much longer lifetime. However, the case with TabChildGlobal is a bit trickier. Smaug thinks that the TabChildGlobal should have a zone and that everything descendant should live in that zone. I think this can probably be done as a followup. Please file and CC the two of us? ::: js/xpconnect/idl/nsIXPConnect.idl @@ +319,1 @@ > */ Needs an IID rev. @@ +324,5 @@ > in nsIPrincipal aPrincipal, > in uint32_t aFlags); > > + nsIXPConnectJSObjectHolder > + initClassesWithNewWrappedGlobalInZone( I'm not very happy about this. Can we make initClassesWithNewWrappedGlobal just take a ZoneSpecifier? ::: js/xpconnect/src/XPCComponents.cpp @@ +3278,5 @@ > nsIPrincipal *principal = sop->GetPrincipal(); > > JSObject *sandbox; > > + sandbox = xpc::CreateGlobalObject(cx, &SandboxClass, principal, JS::SystemZone); This is going to be pretty suboptimal in a lot of cases, and sandboxes account for an ever-growing portion of our compartments. Can you add a ZoneSpec to SandboxOptions, and parse it out of a "sameZoneAs" property in ParseOptionsObject? This should be very easy to do, as all of the existing machinery is there. It's fine to do as a separate patch, but I'd like the functionality to be a part of this landing so that Jetpack & co can start taking advantage of zones immediately. What's the impact of something with page-lifetime ending up with the system zone? I'm assuming that it isn't an issue for correctness, but will it significantly increase fragmentation? ::: netwerk/base/src/ProxyAutoConfig.cpp @@ +529,5 @@ > NS_ENSURE_TRUE(mContext, NS_ERROR_OUT_OF_MEMORY); > > JSAutoRequest ar(mContext); > > + mGlobal = JS_NewGlobalObject(mContext, &sGlobalClass, nullptr, JS::SystemZone); SystemZone is per-runtime, right? Because this stuff is all happening in an off-main-thread runtime...

I'll respond to the specific comments tomorrow. In the meantime, here's a rolled up patch for Nick to test.

This addresses the API issue.

I think this is what you wanted.

I forgot to post this before. This is what fixes the problem where there were lots of decommitted arenas. Previously, the isSystem flag was being set inconsistently and causing us to use the wrong kind of chunk. When I started the zone patches, I put the isSystem flag on the zone. I thought it needed to be there because we use it to decide whether a new arena should be allocated in a system chunk or a non-system chunk, and arenas are now per-zone. However, isSystem doesn't really fit on the zone because a zone can contain a mixture of system compartments and non-system compartments. So it seemed like it would be simpler to put isSystem back on the compartment where it belongs. However, I added another isSystem flag to the zone. This is needed in the few cases (like the decision about which kind of chunk to use) where we only have a zone and not a compartment.

mccr8 said: > Is compartment nuking probably going to wipe out the entire tab zone when we close the tab? Nuking is still compartment-based. I think in practice we'll nuke all the compartments in the tab's zone, but I haven't checked. njn said: > Here, a zone's size is measured as just the zone-specific data; the > compartment sizes aren't put under the zone. In one sense they don't need to > be, because (unlike the "js-non-window" case) there's only one zone, so we know > which zone the two compartments belong to. But to be consistent with > "js-non-windows" the compartments would be reported under the zone, like this: I changed the worker naming as you recommended. However, keep in mind that workers have multiple zones. One for atoms and two others for globals. I don't know why there are two globals. bholley said: > So, |top| here will be an outer window, which means that GetGlobalJSObject() will return the > outer window proxy. This object isn't a global object in the JS sense, though it should be in > the compartment of the current global. Is that enough? Yeah, you can use any object as long as it's in the right zone. > However, the case with TabChildGlobal is a bit trickier. Smaug thinks that the TabChildGlobal > should have a zone and that everything descendant should live in that zone. I think this can > probably be done as a followup. Please file and CC the two of us? I'm not sure I understand this. You want the TabChildGlobal to go in the same zone as the top-level window? In other words, what exactly is "everything descendant"? And if so, I'm not sure how I would detect this. Is there a way to get the TabChildGlobal from nsGlobalWindow? > What's the impact of something with page-lifetime ending up with the system zone? I'm assuming > that it isn't an issue for correctness, but will it significantly increase fragmentation? It's not a correctness issue. It will increase fragmentation. The significance depends on when objects are allocated and how many of them there are. > SystemZone is per-runtime, right? Because this stuff is all happening in an off-main-thread > runtime... Each runtime has its own system zone, so this is fine.

> However, I added another isSystem flag to the zone. This is needed in the > few cases (like the decision about which kind of chunk to use) where we only > have a zone and not a compartment. Hmm. Most of the compartments that are currently reported under "js-non-window" end up in a single zone -- presumably that zone is marked as isSystem=true? If so, I think that should capture most of what's important. It would also be good if the zones holding browser.xul and the hidden window are marked as isSystem=true -- does that happen? Maybe it does thanks to the FreshZone stuff, because the first compartment in the zone is a system compartment so the zone is marked as a system zone? BTW, I'm bamboozled by SameZoneAs() -- you're casting from a JSObject* to a ZoneSpecifier?

Comment on attachment 716302 [details] [diff] [review] 23. Add per-compartment isSystem flag Review of attachment 716302 [details] [diff] [review]: ----------------------------------------------------------------- This patch seems ok, but I'd like clarification on comment 63.

(In reply to Bill McCloskey (:billm) from comment #62) he case with TabChildGlobal is a bit trickier. Smaug thinks that the TabChildGlobal > > should have a zone and that everything descendant should live in that zone. I think this can > > probably be done as a followup. Please file and CC the two of us? > > I'm not sure I understand this. You want the TabChildGlobal to go in the > same zone as the top-level window? In other words, what exactly is > "everything descendant"? Docshells form a tree, and nodes are labeled as either "content" or "chrome". In general, the root of the tree begins as chrome, and then at some point towards the leaves the nodes switch to being content. That switch is a chrome-content boundary, and is detected when content gets |top|. That is to say: in content, |top| will always return the deepest ancestor whose type is still "content". In general, the chrome-content boundary also describes the lifetime boundary - the content |top| is also the eldest ancestor with page lifetime. But in the TabChildGlobal case, the parent node of the content |top| also has page lifetime. So we should probably put it in the content zone. > And if so, I'm not sure how I would detect this. Is there a way to get the > TabChildGlobal from nsGlobalWindow? It's tricky, because depending on the setup there may not be a TabChildGlobal at all. But smaug thinks we should add such an API, and use it. In particular, we should check if the nsGlobalWindow is descended from a TabChildGlobal, and if so, use the TabChildGlobal's zone. Otherwise, we do what the current patch does. Make sense? File a bug?

> |top| will always return the deepest ancestor whose type is still "content". Except when browserframe is involved.

Comment on attachment 716298 [details] [diff] [review] 20. Group compartments into zones by tab (v. 2) Review of attachment 716298 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley

Comment on attachment 716300 [details] [diff] [review] 22. add a parameter to specify which zone a sandbox is placed in Review of attachment 716300 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure you need the sameZoneAs object at some point, otherwise it'll always just be a proxy in the caller's compartment. Unless JS_NewGlobalObject handles that somehow?

Fixed.

Comment on attachment 716814 [details] [diff] [review] 22. add a parameter to specify which zone a sandbox is placed in (v2) Review of attachment 716814 [details] [diff] [review]: ----------------------------------------------------------------- In XPCWrappedNativeScope::EnsureXBLScope, please set: options.sameZoneAs = global; Do we have any way of testing this? Please let the jetpack folks know what this is about and how to use it. r=bholley with all that

> Hmm. Most of the compartments that are currently reported under "js-non-window" end up in > a single zone -- presumably that zone is marked as isSystem=true? If so, I think that > should capture most of what's important. It would also be good if the zones holding > browser.xul and the hidden window are marked as isSystem=true -- does that happen? Yes, that happens. Right now browser.xul is in the system zone and the hidden window gets its own zone, which also has isSystem=true. This happens because the hidden window compartment is initially created with system principals and then its principals are somehow downgraded with JS_SetCompartmentPrincipals. However, the zone keeps the isSystem=true flag since JS_SetCompartmentPrincipals doesn't affect the zone. > BTW, I'm bamboozled by SameZoneAs() -- you're casting from a JSObject* to a ZoneSpecifier? The ZoneSpecifier is a tagged pointer. 0 means FreshZone, 1 means SystemZone, and anything else means it gets put in the zone of that JSObject*. So we're relying on 0 and 1 not being valid pointers. > In general, the chrome-content boundary also describes the lifetime boundary - the content > |top| is also the eldest ancestor with page lifetime. But in the TabChildGlobal case, the > parent node of the content |top| also has page lifetime. So we should probably put it in the > content zone. I was playing around with this today and I realized that browser.xul is actually created by InitTabChildGlobalInternal. So it does seem like I should figure out how to put that in the same zone as the rest of the page-level stuff. However, I still don't understand the relationship. How does one convert between a docshell, an nsGlobalWindow, and whatever creates the TabChildGlobal?

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ede352670cd I added a small test for the sandbox creation thing. Who should I talk to about Jetpack?

(In reply to Bill McCloskey (:billm) from comment #72) > Who should I talk to about Jetpack? Dave Townsend should be able to point you in the right direction.

Could this have caused a significant performance degradation, possibly debug-only? Windows debug reftests are red due to timeouts (unable to complete run in less than 2 hours) since this landed.

Sorry, but I backed this out from inbound, on suspicion of causing Win Debug reftest perma-red (timeout). https://hg.mozilla.org/integration/mozilla-inbound/rev/181787e9d670 Other Windows reftest runs seem to be taking significantly longer, too (e.g. both Win7 and WinXP Opt runs increased by about 5-10 minutes), so it's not purely a debug-only issue.

One thing to investigate is what CC times look like. When I was doing the initial compartment merging work, mochitest browser chrome would get ridiculous CC times due to (I assume) leakiness.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #73) > (In reply to Bill McCloskey (:billm) from comment #72) > > Who should I talk to about Jetpack? > > Dave Townsend should be able to point you in the right direction. Alex is your man and I filed bug 844180 to track adding support for this to the Jetpack SDK.

(In reply to Jonathan Kew (:jfkthame) from comment #75) > Sorry, but I backed this out from inbound, on suspicion of causing Win Debug > reftest perma-red (timeout). > > https://hg.mozilla.org/integration/mozilla-inbound/rev/181787e9d670 > > Other Windows reftest runs seem to be taking significantly longer, too (e.g. > both Win7 and WinXP Opt runs increased by about 5-10 minutes), so it's not > purely a debug-only issue. Though debug was 40%+ slower (went from 98-103 minutes up to 141-145 minutes). (I say "+" since there's overhead of downloading builds and tests in those times.) And the backout did indeed fix the slowdown; see https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Rev3%20WINNT%206.1%20mozilla-inbound%20debug%20test%20reftest

I submitted a patch to the SDK in order to use this feature (bug 844180), it looks like the win is big. And I tested against inbound build, no particular crashes nor test fail in SDK test suite.