605662 - (compartmentGC) [meta] enable per-compartment GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

Enable per-compartment GCs.

Comment 1

[Andreas Gal :gal]

Blocks a blocker.

Comment 2

[Andreas Gal :gal]

I can't mark bug 584860 as a dependency due to some cyclic dependency issue.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> I can't mark bug 584860 as a dependency due to some cyclic dependency issue.

Sorted out, I see ;-).

Who should take assignment of this metabug?

/be

Comment 4

[Andreas Gal :gal]

I will take the bug until someone else steps forward.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: running v8 benchmarks in the browser with single tab

The current situation shows that marking is about 30 - 50% of the overall GC time when we run v8 in the browser.

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: running v8 benchmarks in the browser with multiple open tabs

Here I opened about 10 tabs with popular payloads and afterwards I were running v8. The numbers show that the marking time increases and decrease the benchmark score from about 5000 to 4600. With per-compartment GC we should be able to see the same GC pause time as before.

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: running v8 benchmarks in the browser with multiple open tabs

and now with the right file type.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: WIP patch

This patch does the following:

1. Allows a single compartment to be GC'd, rather than all compartments. The code for this is in MarkAndSweepCompartment. During a compartment GC, script filenames and shapes are not swept. A full GC is needed to do this.

2. During a compartmental GC, avoids marking objects in other compartments. This is done with a simple check in the marking code. For this to work properly, we must ensure that all cross-compartment pointers are listed in the crossCompartmentWrappers field of a compartment. Otherwise we may collect live objects.

3. Changes JS_IsAboutToBeFinalized to return false if the argument is in a different compartment from the one being collected during a compartmental GC (since we don't mark those objects).

4. Changes the condition for GCing in MaybeGC. I found that with a single tab, MaybeGC was triggering during SunSpider as we want. But with multiple tabs it wasn't. The new check is compartment-relative, so it shouldn't be affected by other tabs.

-- Performance --

GC latency: To measure this, I ran SunSpider and measured the average time each GC takes. I used two configurations: one with SunSpider alone, and the other with 5 other tabs open (GMail, Zimbra, etc.). "Full" means without the patch, "Compartment" means a compartmental GC with the patch applied.

Full, 1 tab: 31ms
Full, 6 tabs: 79ms
Compartment, 1 tab: 23ms
Compartment, 6 tabs: 27ms

Our SunSpider score also improves. This is mostly due to the MaybeGC change, which causes us to GC outside of SunSpider's timing loop. On my machine, I get these numbers:

Full, 1 tab: 206ms
Full, 6 tabs: 259ms
Compartment, 1 tab: 208ms
Compartment, 6 tabs: 212ms

I think the difference between the single-tab configurations is noise. The "Full" case gets slower because it stops getting MaybeGC calls, so it starts doing last ditch GCs inside the timing loop. I'm not sure why the "Compartment" case gets slower. It doesn't seem to be related to GC. Maybe just that it takes a little longer to search various global caches.

Finally, the V8 benchmark. We actually run GCs during the timing loop here, so GC cost is more important. Here are the scores (higher is better).

Full, 1 tab: 4237
Full, 6 tabs: 3618
Compartment, 1 tab: 4463
Compartment, 6 tabs: 4380

We're clearly better here. Again, we lose a little in the "Compartment" case with more tabs. I assume this is for the same reason as in SunSpider. I'll look into this more.

-- Caveats --

1. In normal browsing, you probably won't notice much difference. Most GCs are cycle collections, which are necessarily full GCs that happen during idle time. You might notice something on JS-heavy page, though, like an animation or a game.

2. Compartmental GCs don't collect all the garbage. First, we don't collect any garbage in other compartments. Second, cross-compartment pointers are treated as roots, so we don't even collect all the garbage in the compartment we're collecting. This may affect performance since it hurts cache locality, requires more marking in the future, etc.

Comment 9

[David Mandelin [:dmandelin]]

Very nice. One thing I have been concerned about for a while is that if someone runs SunSpider with many tabs open, they will get a much worse score than otherwise, so they won't see how fast we are now and how much we've improved. This seems to almost completely fix that. The V8 bump is great, too.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: updated patch

Here's an updated version of the patch that fixes some bugs.

I pushed it to the try server:
http://hg.mozilla.org/try/rev/94e7bcb0cdf5

Unfortunately, there seems to be some orange. I think the next step is probably to play around with gcZeal to get it to trigger compartmental GCs often. Also, it might be a good idea to put in asserts that ensure that there are no unexpected cross-compartment pointers. I tried to do this, but I had difficulty figuring out which cross-compartment pointers are legal. Maybe it would be a good idea just to check any cross-compartment pointers to ensure that they're in the crossCompartmentWrappers maps.

An additional thing to look at is the following. Originally, I thought it would be okay to call MarkContext only on contexts associated with the compartment being GC'd. However, I couldn't even get the browser to start this way, so I just changed the code to mark every context. I never had time to investigate what was going on. Marking all contexts may just be hiding a deeper problem.

Comment 11

[Andreas Gal :gal]

Moved blocking bugs into a post-2.0 bug. This bug has no more dependencies with the current approach (still synchronize all threads, even if we just GC one compartment).

Comment 12

[Gregor Wagner [:gwagner]]

Attachment: High throughput benchmark Global GC

I did some measurements with high throughput benchmarks. This are the numbers for the current Global GC for http://29a.ch/2010/6/2/realtime-raytracing-in-javascript.

Comment 13

[Gregor Wagner [:gwagner]]

Attachment: High throughput Compartment GC

There is even a 20% win if we only have a single compartment with high allocation rate.

Comment 14

[Gregor Wagner [:gwagner]]

Attachment: patch

update.
Add an assertion that no things of other compartments are marked during compartment gc.
Fixed marking dependent strings.

Comment 15

[Gregor Wagner [:gwagner]]

TryServer looks green. 
For a last test I want to replace the global GC with an iteration over all compartments and perform only per-compartment GCs. 
If this works out it is good to go.

Comment 16

[Andreas Gal :gal]

It should work, but #15 will leak.

Comment 17

[Gregor Wagner [:gwagner]]

(In reply to comment #16)
> It should work, but #15 will leak.

Yeah this was just for GC hazards.

Right now we don't support js_GCThingIsMarked for per-compartment GC because objects outside of the involved compartment are never marked and GetFlatJSObjectAndMark can mark an object without going through the standard marking procedure.

Comment 18

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 19

[Andreas Gal :gal]

Comment on attachment 496503 [details] [diff] [review]
patch

> struct JSRuntime {
>     /* Default compartment. */
>     JSCompartment       *defaultCompartment;
> #ifdef JS_THREADSAFE
>     bool                defaultCompartmentIsLocked;
> #endif
> 
>+    JSCompartment       *gcCompartment;
>+    

Not a very expressive name.

>     /* List of compartments (protected by the GC lock). */
>     js::WrapperVector compartments;
> 
>     /* Runtime state, synchronized by the stateChange/gcLock condvar/lock. */
>     JSRuntimeState      state;
> 
>     /* Context create/destroy callback. */
>     JSContextCallback   cxCallback;
>@@ -1317,18 +1319,19 @@ struct JSRuntime {
>     size_t              gcTriggerBytes;
>     size_t              gcLastBytes;
>     size_t              gcMaxBytes;
>     size_t              gcMaxMallocBytes;
>     uint32              gcEmptyArenaPoolLifespan;
>     uint32              gcNumber;
>     js::GCMarker        *gcMarkingTracer;
>     uint32              gcTriggerFactor;
>-    volatile JSBool     gcIsNeeded;
>-
>+    volatile bool       gcIsNeeded;
>+    JSCompartment       *gcIsNeededCompartment;

Why do we need 2 here? is gcCompartment not enough?

>+
>+bool
>+checkArenaListAllUnmarked(JSCompartment *comp) {
>+    if (comp->arenas[FINALIZE_OBJECT0].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_OBJECT2].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_OBJECT4].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_OBJECT8].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_OBJECT12].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_OBJECT16].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_FUNCTION].markedThingsInArenaList() ||
>+#if JS_HAS_XML_SUPPORT
>+        comp->arenas[FINALIZE_XML].markedThingsInArenaList() ||
>+#endif
>+        comp->arenas[FINALIZE_STRING].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_EXTERNAL_STRING].markedThingsInArenaList() ||
>+        comp->arenas[FINALIZE_SHORT_STRING].markedThingsInArenaList()) {
>+            return false;
>+    }
>+    return true;
>+}

Return a || b || c || d ...

>+    for (JSCompartment **c = compartments.begin(); c != compartments.end(); ++c) {
>+        JSCompartment *comp = *c;
>+        comp->setGCLastBytes(gcLastBytes);

Just (*c)-> seems shorter and to the point.

>     do {
>         for (;;) {
>             if (JSString::isStatic(str))
>                 break;
>             JS_ASSERT(JSTRACE_STRING == GetFinalizableTraceKind(str->asCell()->arena()->header()->thingKind));
>+            JS_ASSERT(thing->asCell()->compartment() == str->asCell()->compartment() ||
>+                      str->asCell()->compartment() == rt->defaultCompartment);
>+            /* If we perform single-compartment GC don't mark String outside the current compartment */
>+            if (rt->gcCompartment && str->asCell()->compartment() != rt->gcCompartment) {
>+                JS_ASSERT(str->asCell()->compartment() == rt->defaultCompartment);
>+                break;
>+            }

Dependent strings should mix. Do we really do that?

>             if (!str->asCell()->markIfUnmarked())
>                 break;
>             if (!str->isDependent())
>                 break;
>             str = str->dependentBase();
>         }
>         str = iter.next();
>     } while (str);
>diff -r 4d97e9955bfb js/src/jspropertytree.cpp
>--- a/js/src/jspropertytree.cpp	Thu Dec 09 00:16:30 2010 -0500
>+++ b/js/src/jspropertytree.cpp	Thu Dec 09 08:39:16 2010 -0800
>@@ -889,8 +889,27 @@ js::PropertyTree::sweepShapes(JSContext 
>                 putc('\n', dumpfp);
>             }
> 
>             fclose(dumpfp);
>         }
>     }
> #endif /* DEBUG */
> }
>+
>+void
>+js::PropertyTree::unmarkShapes(JSContext *cx)
>+{
>+    JSArena **ap = &JS_PROPERTY_TREE(cx).arenaPool.first.next;
>+    while (JSArena *a = *ap) {
>+        Shape *limit = (Shape *) a->avail;
>+
>+        for (Shape *shape = (Shape *) a->base; shape < limit; shape++) {
>+            /* If the id is null, shape is already on the freelist. */
>+            if (JSID_IS_VOID(shape->id))
>+                continue;
>+
>+            if (shape->marked())
>+                shape->clearMark();
>+        }
>+        ap = &a->next;
>+    }
>+}

Why is this unmarking necessary? Because we mark some stuff and we have no way to clear mark bits in shapes and rely on disposing them in case of a full GC?

>+void
>+js_UnmarkScriptFilenames(JSRuntime *rt)
>+{
>+    if (!rt->scriptFilenameTable)
>+        return;
>+
>+    JS_HashTableEnumerateEntries(rt->scriptFilenameTable,
>+                                 js_script_filename_unmarker,
>+                                 rt);
>+}
>+

I wish we could do this differently, i.e. with a flag?

Comment 20

[Gregor Wagner [:gwagner]]

> >-
> >+    volatile bool       gcIsNeeded;
> >+    JSCompartment       *gcIsNeededCompartment;
> 
> Why do we need 2 here? is gcCompartment not enough?

gcIsNeededCompartment is used for saving the compartment that has to perform a per-compartment GC and it is set to NULL once more compartments need a GC.
gcCompartment is set once we actually start a GC for a certain compartment. We don't want anybody to change this during a GC. So rt->gcCompartment indicates that we are currently performing a per-compartment GC.


> 
> >     do {
> >         for (;;) {
> >             if (JSString::isStatic(str))
> >                 break;
> >             JS_ASSERT(JSTRACE_STRING == GetFinalizableTraceKind(str->asCell()->arena()->header()->thingKind));
> >+            JS_ASSERT(thing->asCell()->compartment() == str->asCell()->compartment() ||
> >+                      str->asCell()->compartment() == rt->defaultCompartment);
> >+            /* If we perform single-compartment GC don't mark String outside the current compartment */
> >+            if (rt->gcCompartment && str->asCell()->compartment() != rt->gcCompartment) {
> >+                JS_ASSERT(str->asCell()->compartment() == rt->defaultCompartment);
> >+                break;
> >+            }
> 
> Dependent strings should mix. Do we really do that?

Dependent strings don't mix and they can't be in the defaultCompartment. The base has to be in the current or the defaultCompartment if I understand correctly.

> >+
> >+void
> >+js::PropertyTree::unmarkShapes(JSContext *cx)
> >+{
> >+    JSArena **ap = &JS_PROPERTY_TREE(cx).arenaPool.first.next;
> >+    while (JSArena *a = *ap) {
> >+        Shape *limit = (Shape *) a->avail;
> >+
> >+        for (Shape *shape = (Shape *) a->base; shape < limit; shape++) {
> >+            /* If the id is null, shape is already on the freelist. */
> >+            if (JSID_IS_VOID(shape->id))
> >+                continue;
> >+
> >+            if (shape->marked())
> >+                shape->clearMark();
> >+        }
> >+        ap = &a->next;
> >+    }
> >+}
> 
> Why is this unmarking necessary? Because we mark some stuff and we have no way
> to clear mark bits in shapes and rely on disposing them in case of a full GC?

We clear the marking bit only during sweepShapes and we removed this call from MarkAndSweepCompartment. So they would be marked once we start the next GC.
Maybe we could avoid marking and sweeping? 
There is a IsShapeAboutToBeFinalized function that needs the information in the tracer. How "wrong" would it be to just always return false if we perform a per-compartment GC?

> 
> >+void
> >+js_UnmarkScriptFilenames(JSRuntime *rt)
> >+{
> >+    if (!rt->scriptFilenameTable)
> >+        return;
> >+
> >+    JS_HashTableEnumerateEntries(rt->scriptFilenameTable,
> >+                                 js_script_filename_unmarker,
> >+                                 rt);
> >+}
> >+
> 
> I wish we could do this differently, i.e. with a flag?

I removed this now and just don't mark and sweep scriptFileNames during per-compartment GC. I don't see any side-effects but maybe I am missing something?

Comment 21

[Gregor Wagner [:gwagner]]

Attachment: update

Comment 22

[Gregor Wagner [:gwagner]]

Attachment: running v8 benchmarks in the browser with single tab (Per-Compartment GC enabled)

Comment 23

[Gregor Wagner [:gwagner]]

Attachment: running v8 benchmarks in the browser with multiple open tabs (Per-Compartment GC enabled)

Multiple tabs with common pages and running v8 benchmarks in a tab. The comparison to the old approach shows a GC pause time reduction of up to 80%!

Comment 24

[Gregor Wagner [:gwagner]]

I also measured the overhead for the extra rt->gcCompartment check in the marking path.
The cylces for marking a linked list with 1400000 objects increases from 204 to 214 mill cycles.

Comment 25

[Gregor Wagner [:gwagner]]

Attachment: update

Comment 26

[Gregor Wagner [:gwagner]]

Attachment: update

Fix a problem where we could end up with a compartment-GC when we call JS_GC.

Comment 27

[Andreas Gal :gal]

Comment on attachment 497621 [details] [diff] [review]
update

Compartment contains rt, no need to pipe cx into IsAboutToBeFinalized.
No {} in setgCTriggerFactor, its just one line.

Looks good otherwise. Will stamp with cx not passed through.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Any chance to get an updated patch which applies cleanly to trunk. 
I'd be happy to test this (and interruptible CC) in my daily browsing.

Comment 29

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 30

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 31

[Daniel C]

(In reply to comment #30)
> Fixing fields my automated script accidentally blanked. Apologies for the
> bugspam

Doesn't look right to me. You've just re-removed a load of people from the CC list.

Comment 32

[Gregor Wagner [:gwagner]]

Attachment: rebase

Comment 33

[Gregor Wagner [:gwagner]]

Attachment: new version

Comment 34

[Gregor Wagner [:gwagner]]

Attachment: new version

Comment 35

[Gregor Wagner [:gwagner]]

Attachment: update

Comment 36

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/fed63a7f54aa

Comment 37

[Chris Leary [:cdleary] (not checking bugmail)]

gwagner, gal said that there was one remaining issue to root out before migrating the changesets to m-c -- what issue is that?

Removing fixed-in-tracemonkey as a precaution for when I forget about the mystery issue later, feel free to add it back if this is really done landing.

Comment 38

[Andreas Gal :gal]

http://hg.mozilla.org/mozilla-central/rev/8e0b1aaf4acb

Comment 39

[Brendan Eich [:brendan]]

Has bug 612225 had its patch merged from tm to m-c?

I don't think bug 608855 can block this one from being fixed-in-tracemonkey at least for m-c merging purposes.

/be