Closed Bug 605662 (compartmentGC) Opened 10 years ago Closed 9 years ago

enable per-compartment GC

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: gal, Assigned: gwagner)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: meta, perf, Whiteboard: [cib-compartments][hardblocker], fixed-in-tracemonkey)

Attachments

(8 files, 10 obsolete files)

6.84 KB, image/png
Details
8.42 KB, image/png
Details
53.39 KB, patch
Details | Diff | Splinter Review
8.47 KB, image/png
Details
9.94 KB, image/png
Details
7.57 KB, image/png
Details
8.60 KB, image/png
Details
60.73 KB, patch
Details | Diff | Splinter Review
Enable per-compartment GCs.
blocking2.0: --- → ?
Target Milestone: --- → mozilla2.0b8
Blocks: 558861
Blocks a blocker.
I can't mark bug 584860 as a dependency due to some cyclic dependency issue.
Blocks: 606057
No longer blocks: 606057
Depends on: 606057
Depends on: 604813
Depends on: 606059
Depends on: 606060
Depends on: 606064
Depends on: 606065
Depends on: 606068
Depends on: 584860
Summary: enable per-compartment GC → enable per-compartment GC [meta]
Keywords: meta
Summary: enable per-compartment GC [meta] → enable per-compartment GC
(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
I will take the bug until someone else steps forward.
Assignee: general → gal
blocking2.0: ? → beta8+
Depends on: 609104
blocking2.0: beta8+ → beta9+
Depends on: 613319
Depends on: 614578
The current situation shows that marking is about 30 - 50% of the overall GC time when we run v8 in the browser.
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.
and now with the right file type.
Attachment #494481 - Attachment is obsolete: true
Attached patch WIP patch (obsolete) — Splinter Review
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.
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.
Attached patch updated patchSplinter Review
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.
Attachment #494927 - Attachment is obsolete: true
Blocks: 616927
No longer depends on: 604813, 606057, 606059, 606060, 606068, 609104, 613319, 614578, 606064, 606065
No longer depends on: 584860
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).
Assignee: gal → anygregor
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.
There is even a 20% win if we only have a single compartment with high allocation rate.
Attached patch patch (obsolete) — Splinter Review
update.
Add an assertion that no things of other compartments are marked during compartment gc.
Fixed marking dependent strings.
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.
It should work, but #15 will leak.
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #496384 - Attachment is obsolete: true
Attachment #496503 - Flags: review?(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?
> >-
> >+    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?
Attached patch update (obsolete) — Splinter Review
Attachment #496503 - Attachment is obsolete: true
Attachment #496503 - Flags: review?(gal)
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%!
Whiteboard: [cib-compartments]
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.
Attached patch update (obsolete) — Splinter Review
Attachment #496681 - Attachment is obsolete: true
Attachment #496870 - Flags: review?(gal)
Depends on: 608855
Keywords: perf
Attached patch update (obsolete) — Splinter Review
Fix a problem where we could end up with a compartment-GC when we call JS_GC.
Attachment #496870 - Attachment is obsolete: true
Attachment #496870 - Flags: review?(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.
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.
Whiteboard: [cib-compartments] → [cib-compartments][hardblocker]
Depends on: 621845
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)
No longer blocks: 616927, 558861
blocking2.0: beta9+ → betaN+
No longer depends on: 608855, 621845
Keywords: meta, perf
Blocks: 616927, 558861
Depends on: 608855, 621845
Keywords: meta, perf
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
(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.
Depends on: 620028
Depends on: 615260
Alias: compartmentGC
Depends on: 622592
Depends on: 614131
Depends on: 612225
Depends on: 618871
Depends on: 606058
Attached patch rebase (obsolete) — Splinter Review
Attachment #497621 - Attachment is obsolete: true
Attached patch new version (obsolete) — Splinter Review
Attachment #502135 - Flags: review+
Attached patch new version (obsolete) — Splinter Review
Attachment #502113 - Attachment is obsolete: true
Attachment #502135 - Attachment is obsolete: true
Attached patch updateSplinter Review
Attachment #502162 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/fed63a7f54aa
Whiteboard: [cib-compartments][hardblocker] → [cib-compartments][hardblocker], fixed-in-tracemonkey
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.
Whiteboard: [cib-compartments][hardblocker], fixed-in-tracemonkey → [cib-compartments][hardblocker]
Whiteboard: [cib-compartments][hardblocker] → [cib-compartments][hardblocker], fixed-in-tracemonkey
Depends on: 624168
http://hg.mozilla.org/mozilla-central/rev/8e0b1aaf4acb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Depends on: 689269
You need to log in before you can comment on or make changes to this bug.