Closed Bug 609104 Opened 14 years ago Closed 13 years ago

move the property tree into the compartment

Categories

(Core :: JavaScript Engine, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+
status2.0 --- wanted

People

(Reporter: gal, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [hardblocker][fixed-in-tracemonkey][has patch])

Attachments

(1 file, 8 obsolete files)

real fix for orange
93.59 KB, patch
brendan
: review+
Details | Diff | Splinter Review 
If we want to GC per compartment, we might have to move the tree into the compartment.
No longer depends on: 606068
See bug 569422. I think this bug must depend on that one.

/be
Depends on: 569422
blocking2.0: beta8+ → beta9+
No longer depends on: 569422
No longer have to block on this. We will force all threads to synchronize even
if we only GC a single compartment.

This is a 1ms speedup, based on a preliminary patch (0.7% ish on my machine) since it avoids the LOCK/UNLOCK around 8000 shape allocations.
blocking2.0: beta9+ → ---
status2.0: --- → wanted
Blocks: 616927
No longer blocks: compartmentGC
We do need to block on this, as jseward's helgrind results show. See also my comments from the great scope-ectomy:

                            /*
                             * kidp->isChunk() was true, but if we're racing it
                             * may not be by this point. FIXME: thread "safety"
                             * is for the birds!
                             */

                                    // FIXME unsafe race with kidp->is/toChunk() above.
                                    // But this is all going single-threaded soon...

I had assumed this bug was a blocker. Sorry! To make up, I'll take it and fix it quickly (Andreas has a rotted patch).

/be
Assignee: general → brendan
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Andreas suggested I steal this.
Assignee: brendan → wmccloskey
I checked with Brendan. He is ok with you stealing the bug.
Whiteboard: hardblocker
Attached patch patch (obsolete) — — Splinter Review 
Here's what this patch does:
1. Moves the PropertyTree to JSCompartment
2. Moves the empty shapes to JSCompartment
3. Eliminates the special shape numbers given out to empty shapes. Now they are numbered in the standard way, via js_GenerateShape.
4. Moves most of the shape metering info the compartment.
5. Switches cx->compartment to NULL during most of GC. It's not safe to use cx->compartment during GC, so this way we can catch people who do so.
6. Eliminates locking in the property tree.

There are also a few optional changes that I threw in:
7. The code that switches a KidsPtr from a chunk representation to a hash representation used to live in PropertyTree::getChild. I moved it to PropertyTree::insertChild, which makes more sense, I think.
8. Some syntactic changes to simplify the control-flow.

One thing that doesn't change: shape numbers are still drawn from a global space. This means that js_GenerateShape still needs to use an atomic op. It would be nice to fix this, but then I think we'd have to move the property cache to JSCompartment as well. This patch is big enough as it is, so I think we should do that later.

Details on the stuff above:
* Regarding #2: We used to allocate empty shapes when the first context was allocated. That way, we had a context to use for allocating empty shapes. However, we allocate the atomsCompartment well before a context is available. So I had to change a number of allocators to take a JSCompartment rather than a JSContext.

* Regarding #3: I checked with Jeff, and we're both pretty sure that this is safe. There doesn't seem to be any code that relies on the empty shapes having specific numbers. I made the change so that empty shapes in different compartments have different numbers.

* Regarding #5: I found that there's code in jsdbgapi that allocates shapes during the GC sweep phase. Since the compartment for a newly allocated shape comes from cx->compartment, and since this value isn't well-defined during a GC, this is bad. So, during this phase, I temporarily switched cx->compartment to the one being swept. During the rest of the GC, cx->compartment is set to NULL. Luke says this is safe, and it should catch other nasty errors like this.

The code asserts if a shape in one compartment ever points to a shape in another compartment. Everything passes on tryserver.
Attachment #505621 - Flags: review?(brendan)
(In reply to comment #6)
> 6. Eliminates locking in the property tree.

Cool, is this locking rare or does it give any observable speedup on benchmarks?
Its not rare. I saw 8000 atomic ops for SS alone. Considering the time we run SS in these days it might make a small dent.
(In reply to comment #6)
> 2. Moves the empty shapes to JSCompartment

This is necessary so the shapes will be unique for the property cache, right? This means that when we move the cache to the compartment, we could make the empty shapes compile-time constants. But then perhaps we should move the property cache to the compartment first and simplify the patch?

> However, we allocate the atomsCompartment well before a context is available.
> So I had to change a number of allocators to take a JSCompartment rather than a
> JSContext.

why not change the allocation of atom compartment to happen during the first cx creation?
Moving the tree into the compartment is a blocker. Moving the cache is an improvement so lets stick with the patch as is. Changing the allocation time of the atoms compartment might work. Gregor, what do you think?
(In reply to comment #6)
> Created attachment 505621 [details] [diff] [review]
> patch

This appears to fix the races referred to in comment #3, or at least I
can't detect them any more, whereas I could detect them pre-patch.

However, it also seems to be leaking now, and I'm pretty sure it
wasn't before.  Do you see leaks like this?

65,584 (48 direct, 65,536 indirect) bytes in 2 blocks are definitely lost in loss record 383 of 383
   at 0x4C27878: malloc (vg_replace_malloc.c:236)
   by 0x4D5E11: js_malloc (jsutil.h:209)
   by 0x4D5F28: js::Shape::hashify(JSRuntime*) (jscntxt.h:1372)
   by 0x4D6300: js::Shape::newDictionaryList(JSContext*, js::Shape**) (jsscope.cpp:641)
   by 0x4D631E: JSObject::toDictionaryMode(JSContext*) (jsscope.cpp:649)
   by 0x4D6665: JSObject::addPropertyInternal(JSContext*, long, int (*)(JSContext*, JSObject*, long, js::Value*), int (*)(JSContext*, JSObject*, long, js::Value*), unsigned int, unsigned int, unsigned int, int, js::Shape**) (jsscope.cpp:812)
   by 0x4D6C68: JSObject::putProperty(JSContext*, long, int (*)(JSContext*, JSObject*, long, js::Value*), int (*)(JSContext*, JSObject*, long, js::Value*), unsigned int, unsigned int, unsigned int, int) (jsscope.cpp:896)
   by 0x48C1F0: js_SetPropertyHelper(JSContext*, JSObject*, long, unsigned int, js::Value*, int) (jsobj.cpp:5648)
   by 0x48C3CE: js_SetProperty(JSContext*, JSObject*, long, js::Value*, int) (jsobj.cpp:5685)
   by 0x5D1D10: void js::mjit::stubs::SetElem<0>(js::VMFrame&) (jsobj.h:1199)
   by 0xEBA0A27: ???
   by 0x5552B0: js::mjit::EnterMethodJIT(JSContext*, JSStackFrame*, void*, js::Value*) (MethodJIT.cpp:748)
Thanks, Julian. I'll look into this. It gives me some motivation to try and understand dictionary mode.

Do you see these leaks in browser, or in some shell test?
(In reply to comment #12)
> Do you see these leaks in browser, or in some shell test?

The leak I showed was with a parallel workers shell test I used to do
the race testing.  But in fact any run of the shell leaks, for
example:

echo 1 | valgrind --smc-check=all --leak-check=full ./R64/js -j -m -p
(In reply to comment #9)
> This is necessary so the shapes will be unique for the property cache, right?
> This means that when we move the cache to the compartment, we could make the
> empty shapes compile-time constants. But then perhaps we should move the
> property cache to the compartment first and simplify the patch?

I think it's an eventual goal for Shapes to be GCed just like JSObjects. For that, we need all shapes to be compartment-local anyway (in order to avoid ugly special cases in the GC). So I think that the move is warranted regardless.

> why not change the allocation of atom compartment to happen during the first cx
> creation?

Looking over the patch, it wasn't too much of a hassle to change a few allocators to take a JSCompartment rather than a JSContext.
(In reply to comment #10)
> Moving the tree into the compartment is a blocker. Moving the cache is an
> improvement so lets stick with the patch as is. Changing the allocation time of
> the atoms compartment might work. Gregor, what do you think?

Shouldn't be a problem. We can allocate it during the creation of the first context. Right before we call js_InitCommonAtoms.
Bill, want to incorporate 15? And also, with the tree now being single-threaded, you can greatly simplify the list mode and just use a tagged vector (single entry, or tagged pointer to a Vector<>). No need for the complex chunk magic. That might be worth the code change since it rips out a ton of code. Up to you.
(In reply to comment #16)
> Bill, want to incorporate 15? 

This doesn't really seem worth it to me, but I can do it if it's wanted.

> And also, with the tree now being
> single-threaded, you can greatly simplify the list mode and just use a tagged
> vector (single entry, or tagged pointer to a Vector<>). No need for the complex
> chunk magic. That might be worth the code change since it rips out a ton of
> code. Up to you.

I really like that idea. I'll take out the chunks and the hash table and replace them with a vector.
Chunks aren't needed, they were part of the racy-duplicate-insertion-tolerated thread-safe design (prior to unsafe races).

Don't take out hashing, or you'll make O(n^2) growth bugs.

/be
(In reply to comment #18)
> Don't take out hashing, or you'll make O(n^2) growth bugs.

Could you say more about this? Now that we're single-threaded, we should never have duplicate children. So it seems like we never do any lookups in the hashtable--we only iterate over it.
We might consider the inverse though. Take out everything but a tagged hash table (single entry or a hash table pointer). Also, we can use our hashtable template instead of the custom code.
(In reply to comment #18)
> Don't take out hashing, or you'll make O(n^2) growth bugs.

Never mind, I totally misread this. There are lookups.
It seems like it would be generally useful to build a reusable ProbablySmallHashTable based on HashTable which abstracted the pointer-to-list-to-table trickery behind a regular hash table interface.
Attached patch new patch (obsolete) — — Splinter Review 
I updated the patch to remove the chunk stuff, fix the leak, fix the problem Paul found in bug 626492, and add some extra assertions.
Attachment #505621 - Attachment is obsolete: true
Attachment #505994 - Flags: review?(brendan)
Andreas was right about the speedup. It gets a 5ms boost on Sunspider on my machine.
Comment on attachment 505994 [details] [diff] [review]
new patch


>+    /*
>+     * Switch to the same compartment as the watch point, since changeProperty, below,
>+     * needs to have a compartment.
>+     */
>+    SwitchToCompartment sc(cx, wp->object);
>+

SwitchCompartment is really lame and unsafe. What does changeProperty do that it needs a compartment for? And why does the compartment not already match? Can you show me the stack?
> 
>+    /*
>+     * We should not be depending on cx->compartment in the GC, so set
>+     * it to NULL to look for violations.
>+     */
>+    SwitchToCompartment(cx, (JSCompartment *)NULL);
>+    <--- extra space at the end of the line, look for more cases like this

I like the switch to NULL a lot.

The shape pool grows in increments of 256 shapes. I guess thats ok per compartment.
(In reply to comment #25)
> SwitchCompartment is really lame and unsafe. What does changeProperty do that
> it needs a compartment for? And why does the compartment not already match? Can
> you show me the stack?

See comment 6, under "Regarding #5". Basically, this code is called during GC. It allocates shapes, and now shape allocation gets the new shape's compartment from cx->compartment.

I agree that this is sketchy. I'm actually a little worried that the changeProperty could run code through some sort of setter or something. Is that possible? If so, we need to fix it somehow. We shouldn't be calling into JS from a GC.
Actually looks like this is safe. changeProperty() operates on the scope, not on the object per se, so it won't trigger anything unwanted afaict. The SwitchCompartment should be safe for now.
Comment on attachment 505994 [details] [diff] [review]
new patch

Looks great. Leaving the ? for brendan in case he wants to review too.
Attachment #505994 - Flags: review+
Re: comment 26: changeProperty does not run setters.

bhackett's type inference patch will need some rebasing.

/be
Status: NEW → ASSIGNED
Comment on attachment 505994 [details] [diff] [review]
new patch

> void
> JSCompartment::mark(JSTracer *trc)
> {
>-    for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront())
>-        MarkValue(trc, e.front().key, "cross-compartment wrapper");
>+    /* Only mark the cross-compartment wrappers for a compartment GC. */
>+    if (rt->gcCurrentCompartment != NULL) {
>+        for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront())
>+            MarkValue(trc, e.front().key, "cross-compartment wrapper");
>+    }

Unrelated fix? Ok by me, just wanted to check -- Gregor take note.

>@@ -2357,17 +2343,18 @@ MarkAndSweepCompartment(JSContext *cx, J
> 
>     comp->finalizeStringArenaLists(cx);
>     TIMESTAMP(sweepStringEnd);
> 
>     /*
>      * Unmark the runtime's property trees because we don't
>      * sweep them.
>      */
>-    js::PropertyTree::unmarkShapes(cx);
>+    for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c)
>+        (*c)->propertyTree.unmarkShapes(cx);

The comment now lies. If it did not, why (now that the property tree is per-compartment) is a loop over all compartments in the runtime necessary?

>+    /*
>+     * We should not be depending on cx->compartment in the GC, so set
>+     * it to NULL to look for violations.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Nit: can wrap at wm=79 these days.

>+     */
>+    SwitchToCompartment(cx, (JSCompartment *)NULL);

Kudos. Need more of this and less automagic footgunning. But no need to cast NULL, we don't elsewhere and AFAIK it's not required for portability. No prototype-free C with non-zero NULL on a Cyber or Cray ;-).

>+        empty = js::EmptyShape::create(cx->compartment, clasp);

First of several failures to report OOM. EmptyShape::create should take cx not compartment and report OOM using it. More on this below.

>-PropertyTree::newShape(JSContext *cx, bool gcLocked)
>+PropertyTree::newShapeUnchecked()
> {
>     Shape *shape;
> 
>     shape = freeList;
>     if (shape) {
>         shape->removeFree();
>     } else {
>         JS_ARENA_ALLOCATE_CAST(shape, Shape *, &arenaPool, sizeof(Shape));
>+        if (!shape)
>             return NULL;
>     }
>+#ifdef DEBUG
>+    shape->compartment = compartment;
>+#endif
>+
>+    JS_COMPARTMENT_METER(compartment->livePropTreeNodes++);
>+    JS_COMPARTMENT_METER(compartment->totalPropTreeNodes++);
>     return shape;
> }
> 
>+Shape *
>+PropertyTree::newShape(JSContext *cx)
> {
>-    KidsChunk *chunk;
>+    Shape *shape = newShapeUnchecked();
>+    if (shape == NULL)

Prevailing style tests !shape, not == NULL.

>+        JS_ReportOutOfMemory(cx);

This is correct but it obligates the other caller of newShapeUnchecked, EmptyShape::create, to do likewise. In the patch it does not, due to over-reparameterization from cx to comp.

>+static KidsHash *
>+HashChildren(Shape *child1, Shape *child2)

Nit: canonical naming wants kid1 and kid2. Methods names use more formal "child/children", formal param and local var names are informal.

[snip]

Great to lose all that chunks stuff. Did you test with JS_CHECK_SHAPE_THROTTLE, e.g.

$ JS_CHECK_SHAPE_THROTTLE=50 ./jit_test.py ../Darwin_DBG.OBJ/js

? Worth a try.

>+js::PropertyTree::orphanKids(Shape *shape)
> {
>     KidsPointer *kidp = &shape->kids;
> 
>     JS_ASSERT(!kidp->isNull());
> 
>     /*
>-     * Note that JS_PROPERTY_TREE(cx).removeChild(cx, shape) precedes the call
>+     * Note that removeChild(shape) precedes the call

Please rewrap, e.g. vim Q3j with wm=79.

>      * to orphanKids in sweepShapes, below. Therefore the grandparent must have
>      * either no kids left, or else space in chunks or a hash for more than one
>      * kid.

Left-over chunk refs in the comment -- scour for these everywhere.

> class KidsPointer {
>   private:
>     enum {
>         SHAPE = 0,
>-        CHUNK = 1,
>-        HASH  = 2,
>+        HASH  = 1,
>         TAG   = 3

TAG can be 1 now, why not?

>+    PropertyTree(JSCompartment *comp)
>+        : compartment(comp), freeList(NULL) {
>+        PodZero(&arenaPool);
>+    }

Constructor style wants : half-indented (two spaces), and { on its own line underhanging P in the ctor name.

>+js_GenerateShape(JSRuntime *rt)
> {
>-    JSRuntime *rt;
>     uint32 shape;
> 
>-    rt = cx->runtime;
>     shape = JS_ATOMIC_INCREMENT(&rt->shapeGen);
>     JS_ASSERT(shape != 0);
>     if (shape >= SHAPE_OVERFLOW_BIT) {
>         /*
>          * FIXME bug 440834: The shape id space has overflowed. Currently we
>          * cope badly with this and schedule the GC on the every call. But
>          * first we make sure that increments from other threads would not
>          * have a chance to wrap around shapeGen to zero.
>          */
>         rt->shapeGen = SHAPE_OVERFLOW_BIT;
>         shape = SHAPE_OVERFLOW_BIT;
> 
> #ifdef JS_THREADSAFE
>-        Conditionally<AutoLockGC> lockIf(!gcLocked, rt);
>+        AutoLockGC lockIf(rt);
> #endif
>         TriggerGC(rt);
>     }
>     return shape;

With compartment-confined shapes, shapeGen could be compartment-local but for the shared threadData.propertyCache. Moving shapeGen at least into threadData would eliminate the atomic increment overhead. Is it hard to do now? Followup bug at least.

>+    static EmptyShape *create(JSCompartment *comp, js::Class *clasp) {
>+        js::Shape *eprop = comp->propertyTree.newShapeUnchecked();
>         if (!eprop)
>             return NULL;

As noted, missing OOM check due to cx-to-comp unnecessary reparam here.

I'll quickly stamp a new patch (gal should stand clear :-P).

/be
Attachment #505994 - Flags: review?(brendan)
Attachment #505994 - Flags: review+
BTW, I would not move the property cache into the compartment without some care and study. It's big. If too small, it doesn't work well (and we rely on it to trace well, also to Jaeger IIRC). It might be better off in threadData (along with shapeGen).

/be
Attached patch patch v3 (obsolete) — — Splinter Review 
> Unrelated fix? Ok by me, just wanted to check -- Gregor take note.

This doesn't actually change the semantics of anything. Previously we were only calling JSCompartment::mark for full GCs. Now it's called for per-compartment GCs as well so that the empty shapes always get marked.

> The comment now lies. If it did not, why (now that the property tree is
> per-compartment) is a loop over all compartments in the runtime necessary?

I fixed the comment. The goal was just to unmarkShapes on every compartment's property tree.

> Kudos. Need more of this and less automagic footgunning. But no need to cast
> NULL, we don't elsewhere and AFAIK it's not required for portability. No
> prototype-free C with non-zero NULL on a Cyber or Cray ;-).

Unfortunately, SwitchToCompartment is overloaded, so we need the cast.

> First of several failures to report OOM. EmptyShape::create should take cx not
> compartment and report OOM using it. More on this below.

Oops. Thanks. I fixed these.

> $ JS_CHECK_SHAPE_THROTTLE=50 ./jit_test.py ../Darwin_DBG.OBJ/js
> 
> ? Worth a try.

I tried this without finding anything. As an aside, JSObject::checkShapeConsistency has some code at the end starting with |if (throttle == 0)|. It seems dead. Can we take it out?

> With compartment-confined shapes, shapeGen could be compartment-local but for
> the shared threadData.propertyCache. Moving shapeGen at least into threadData
> would eliminate the atomic increment overhead. Is it hard to do now? Followup
> bug at least.

I don't think we can safely move shapeGen to the thread. Compartments can move between threads. If thread T1 has a bunch of shapes with numbers 1..10, and then its compartment moves to thread T2, we don't want to allocate a new shape with shape number 1 (assuming T2->shapeGen == 1).

This seems like yet another argument for the zone concept.
Attachment #505994 - Attachment is obsolete: true
Attachment #506521 - Flags: review?(brendan)
(In reply to comment #32)
> Created attachment 506521 [details] [diff] [review]
> patch v3
> 
> > Unrelated fix? Ok by me, just wanted to check -- Gregor take note.
> 
> This doesn't actually change the semantics of anything. Previously we were only
> calling JSCompartment::mark for full GCs. Now it's called for per-compartment
> GCs as well so that the empty shapes always get marked.

No, calling JSCompartment::mark for full GCs will result in leaks. This is only needed for per-compartment GCs.
> No, calling JSCompartment::mark for full GCs will result in leaks. This is only
> needed for per-compartment GCs.

Yes. That's why this patch adds an extra guard so that JSCompartment::mark only marks cross-compartment wrappers when we're doing a per-compartment GC.
(In reply to comment #34)
> > No, calling JSCompartment::mark for full GCs will result in leaks. This is only
> > needed for per-compartment GCs.
> 
> Yes. That's why this patch adds an extra guard so that JSCompartment::mark only
> marks cross-compartment wrappers when we're doing a per-compartment GC.

Just looked at the patch. Makes sense now.
Comment on attachment 506521 [details] [diff] [review]
patch v3

>@@ -2354,20 +2340,21 @@ MarkAndSweepCompartment(JSContext *cx, J
> 
>     comp->finalizeObjectArenaLists(cx);
>     TIMESTAMP(sweepObjectEnd);
> 
>     comp->finalizeStringArenaLists(cx);
>     TIMESTAMP(sweepStringEnd);
> 
>     /*
>-     * Unmark the runtime's property trees because we don't
>-     * sweep them.
>+     * Unmark each compartment's property tree. We don't sweep these in a
>+     * per-compartment GC.
>      */
>-    js::PropertyTree::unmarkShapes(cx);
>+    for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c)
>+        (*c)->propertyTree.unmarkShapes(cx);

I am still confused. How could per-compartment GC mark shapes in other compartments? Never mind other threads' compartments, which this loop had better be able to skip (or we have data races).

> > Kudos. Need more of this and less automagic footgunning. But no need to cast
> > NULL, we don't elsewhere and AFAIK it's not required for portability. No
> > prototype-free C with non-zero NULL on a Cyber or Cray ;-).
> 
> Unfortunately, SwitchToCompartment is overloaded, so we need the cast.

D'oh! Andreas said SwitchToCompartment was a footgun, so maybe this will go away.

> As an aside,
> JSObject::checkShapeConsistency has some code at the end starting with |if
> (throttle == 0)|. It seems dead. Can we take it out?

Yes, please. I'm not sure what the point of that was. Probably it was meant for the early if (throttle == 0) return case. You could move it there since it is all assertions (need shape, or else s/shape/lastProp/g).

> > With compartment-confined shapes, shapeGen could be compartment-local but for
> > the shared threadData.propertyCache. Moving shapeGen at least into threadData
> > would eliminate the atomic increment overhead. Is it hard to do now? Followup
> > bug at least.
> 
> I don't think we can safely move shapeGen to the thread. Compartments can move
> between threads. If thread T1 has a bunch of shapes with numbers 1..10, and
> then its compartment moves to thread T2, we don't want to allocate a new shape
> with shape number 1 (assuming T2->shapeGen == 1).

Oh, I keep forgetting that compartments migrate but threadData does not. What a botch.

> This seems like yet another argument for the zone concept.

Yes, indeed.

/be
(In reply to comment #32)
> This seems like yet another argument for the zone concept.

No - this can be an argument to move the property cache into the compartment.
(In reply to comment #37)
> (In reply to comment #32)
> > This seems like yet another argument for the zone concept.
> 
> No - this can be an argument to move the property cache into the compartment.

Again, too bloated as a direct-mapped cache. We'd do better to make something allocated to the side of the bytecode dynamically, per get/set/call-site but not over-allocated for max degree of polymorphism for all sites.

Let's not argue about the zone concept here. I started a new thread on dev-tech-js-engine-internals@lists.mozilla.org.

/be
Attached patch patch v4 (obsolete) — — Splinter Review 
I changed the unmarkShapes code to checkShapesAllUnmarked. This is similar to some other check-unmarked stuff that's already in the GC.

I removed the |throttle==0| assertions. I don't think it would be correct to move them to the top of the function. They seem to assert that the shape is invalid (JSID_IS_EMPTY, for example).

I also wanted to point out an assertion in orphanKids that I removed.
-    /*
-     * Note that JS_PROPERTY_TREE(cx).removeChild(cx, shape) precedes the call
-     * to orphanKids in sweepShapes, below. Therefore the grandparent must have
-     * either no kids left, or else space in chunks or a hash for more than one
-     * kid.
-     */
-    JS_ASSERT_IF(shape->parent, !shape->parent->kids.isShape());
The comment sort of makes sense, but I don't see any reason why we rely on this behavior. If you think it should stay, we should make the comment more informative.

Finally, based on your remarks above, I renamed orphanKids to orphanChildren.
Attachment #506521 - Attachment is obsolete: true
Attachment #506817 - Flags: review?(brendan)
Attachment #506521 - Flags: review?(brendan)
Comment on attachment 506817 [details] [diff] [review]
patch v4

(In reply to comment #39)
> Created attachment 506817 [details] [diff] [review]
> patch v4
> 
> I changed the unmarkShapes code to checkShapesAllUnmarked. This is similar to
> some other check-unmarked stuff that's already in the GC.

Do you still want to dump all the shapes for all compartments there?

> I removed the |throttle==0| assertions. I don't think it would be correct to
> move them to the top of the function. They seem to assert that the shape is
> invalid (JSID_IS_EMPTY, for example).

Ah so -- that is trying to assert something about the *first* shape, the empty one at the end of the parent-linked chain starting at lastProp. If that seems worth doing, it's fixable. Deleting is ok too.

> I also wanted to point out an assertion in orphanKids that I removed.
> -    /*
> -     * Note that JS_PROPERTY_TREE(cx).removeChild(cx, shape) precedes the call
> -     * to orphanKids in sweepShapes, below. Therefore the grandparent must
> have
> -     * either no kids left, or else space in chunks or a hash for more than
> one
> -     * kid.
> -     */
> -    JS_ASSERT_IF(shape->parent, !shape->parent->kids.isShape());
> The comment sort of makes sense, but I don't see any reason why we rely on this
> behavior. If you think it should stay, we should make the comment more
> informative.

That comment has the smell of chunks. It is trying to clarify something about the infallibility of shape sweeping: we always have no kids, or chunky kids with space in the chunk. It can all go.

> Finally, based on your remarks above, I renamed orphanKids to orphanChildren.

Thanks for thinking of the children! ;-)

/be
Attachment #506817 - Flags: review?(brendan) → review+
(In reply to comment #40)
> > I changed the unmarkShapes code to checkShapesAllUnmarked. This is similar to
> > some other check-unmarked stuff that's already in the GC.
> 
> Do you still want to dump all the shapes for all compartments there?

No, since checkShapesAllUnmarked is called during per-compartment GC for every compartment except the one being collected. dumpShapes now gets called directly by the GC, for every compartment.
(In reply to comment #41)
> (In reply to comment #40)
> > > I changed the unmarkShapes code to checkShapesAllUnmarked. This is similar to
> > > some other check-unmarked stuff that's already in the GC.
> > 
> > Do you still want to dump all the shapes for all compartments there?
> 
> No, since checkShapesAllUnmarked is called during per-compartment GC for every
> compartment except the one being collected. dumpShapes now gets called directly
> by the GC, for every compartment.

I asked based on the latest patch. It calls (#ifdef DEBUG) dumpShapes for all compartments from MarkAndSweepCompartment.

/be
> I asked based on the latest patch. It calls (#ifdef DEBUG) dumpShapes for all
> compartments from MarkAndSweepCompartment.

Oh, I see. You were asking whether a per-compartment GC should only dump shapes for the compartment being GCed? Yeah, that probably makes more sense. I'll fix.
The extra asserts to check that no cross-compartment shape marking occurs made this die on the try server. I'll investigate tomorrow.
Attached patch latest patch (obsolete) — — Splinter Review 
This is basically ready to go. There was a small bug where we marked too many shapes, but it's fixed. I'm posting this so Julian can run helgrind again.
Attachment #506817 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/7d82892cb8df
Whiteboard: hardblocker → [hardblocker][fixed-in-tracemonkey]
http://hg.mozilla.org/tracemonkey/rev/c5d19264bbfb (backout)

Failures include Linux/opt/xpcshell Win/debug/jsreftest.
Whiteboard: [hardblocker][fixed-in-tracemonkey] → [hardblocker]
(In reply to comment #45)
> Created attachment 507688 [details] [diff] [review]
> latest patch

This removes the proptree races, but still leaks, although not as
badly as the previous version I tried.  eg, with 3d-cube.  A more
extreme case is:

 57,600 (6,400 direct, 51,200 indirect) bytes in 200 blocks are definitely lost in loss record 72 of 72
    at 0x4C27878: malloc (vg_replace_malloc.c:236)
    by 0x4BBFBA: js_malloc (jsutil.h:210)
    by 0x4BC7EB: js::PropertyTree::insertChild(JSContext*, js::Shape*, js::Shape*) 
                 (jspropertytree.cpp:118)
    by 0x4BD9D9: js::PropertyTree::getChild(JSContext*, js::Shape*, js::Shape const&) 
                 (jspropertytree.cpp:239)
    by 0x4D4EF7: js::Shape::getChild(JSContext*, js::Shape const&, js::Shape**) (jsscope.cpp:512)
    by 0x4D8928: js::Bindings::add(JSContext*, JSAtom*, js::BindingKind) (jsscript.cpp:158)
    by 0x4B330E: js::Parser::functionArguments(JSTreeContext&, JSFunctionBox*, JSParseNode**) 
                 (jsscript.h:247)
    by 0x4AC220: js::Parser::functionDef(JSAtom*, js::Parser::FunctionType, unsigned int)
                 (jsparse.cpp:3142)
    by 0x4AC9CD: js::Parser::functionStmt() (jsparse.cpp:3371)
    by 0x4AFD33: js::Parser::statement() (jsparse.cpp:5950)
    by 0x4B2C24: js::Compiler::compileScript(JSContext*, JSObject*, JSStackFram
    by 0x41F44F: CompileFileHelper(JSContext*, JSObject*, JSPrincipals*, unsign
Whiteboard: [hardblocker] → [hardblocker][has patch]
Attached patch patch to fix leaks (obsolete) — — Splinter Review 
This fixes the leaks that Julian found. It seems like a big enough change to need review.

It used to be that compartments didn't have any outgoing pointers of their own. So we could just mark which ones were reachable and then delete the others at the end. When I added the empty shape pointers to the compartments, I left this scheme in place. Unfortunately, it meant that we were marking empty shapes of compartments that were no longer reachable.

This patch changes things so that compartments are marked like any other object. When you discover one, you call its mark method, and that marks its empty shapes.

I'm running jit-tests through valgrind now to look for more leaks. So far I haven't found any.

The tinderbox failures seem to have been unrelated to this patch.
Attachment #507688 - Attachment is obsolete: true
Attachment #508550 - Flags: review?(brendan)
Attached patch dumpShapes fix (obsolete) — — Splinter Review 
This just fixes a problem I found with dumpShapes. If you call it multiple times in the same GC, it just overwrites the file.
Attachment #508550 - Attachment is obsolete: true
Attachment #508621 - Flags: review?(brendan)
Attachment #508550 - Flags: review?(brendan)
(In reply to comment #50)
> Created attachment 508621 [details] [diff] [review]
> dumpShapes fix

LGTM -- no (proptree) races and no leaks.  \o/
(In reply to comment #51)
> LGTM -- no (proptree) races and no leaks.  \o/

Thanks again for all the help.
Comment on attachment 508621 [details] [diff] [review]
dumpShapes fix

>+/*
>+ * This method marks pointers that cross compartment boundaries.  It should
>+ * should only be called by per-compartment GCs, since full GCs naturally
>+ * follow pointers across compartments.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Nits:

* "should should" across line break
* s/only be called/be called only/
* rewrap with wm=79
* French spacing FTW!

Thanks, this patch makes much more sense. My tiny brain should have remembered the crossCompartmentWrappers map.

/be
Attachment #508621 - Flags: review?(brendan) → review+
Comment on attachment 508621 [details] [diff] [review]
dumpShapes fix

>+    js::PropertyTree::dumpShapes(cx);

Final nit: no need for js:: here, right?

/be
Ugh, had to back out again due to orange.

http://hg.mozilla.org/tracemonkey/rev/bfab16ea5cdc
Whiteboard: [hardblocker][fixed-in-tracemonkey] → [hardblocker][has patch]
tryserver is your friend... can't wait to hear what it was.

/be
I discovered one problem. The original patch just unmarked all shapes after a per-compartment GC, since we didn't actually sweep them. The I updated the patch to assert that no shapes outside the compartment being GCed ever got marked. However, I forgot to unmark the shapes in the compartment being GCed.

To fix this, I realized that there's no reason that we can't sweep shapes during a per-compartment GC, since shapes are now per compartment.

However, I can't figure out how this problem could cause the symptoms of the crash. According to the stack traces, we were crashing because rt->atomsCompartment->emptyArgumentShape->id == 0. Normally the id is JSID_EMPTY == 4. The only way I could see it becoming zero is if the shape were swept. But I don't see how that could happen, since we always mark every compartment's empty shapes.

Anyway, I pushed the sweeping fix to the try server. Maybe it will somehow solve the problem by preventing some sort of weird memory corruption.
Should MarkAndSweepCompartment assert that comp != rt->atomsCompartment?

/be
The assertion is a great idea.

Sweeping shapes during per-compartment GC didn't fix the problem, although I'm certainly glad it's fixed.

However, I can now reproduce this locally. We're definitely sweeping the emptyArgumentsShape in the atomsCompartment. I don't know why yet.
Attached patch fix for orange (obsolete) — — Splinter Review 
I think this should fix it.

It turns out the the marked bit for atomsCompartment was not always being unset after GC ended. Strangely, there were two overlapping places where we were attempting to unmark it. The first was in SweepCompartments. However, SweepCompartments intentionally skips over the atomsCompartment.

The second place was at the end of GCUntilDone. Normally, this managed to unmark the atomsCompartment. However, in this particular test, there was a recursive GC, which was setting gcPoke, causing us to do two consecutive GCs inside GCUntilDone. The second GC was started with the mark bit on atomsCompartment still set. Consequently, we weren't marking any of the empty shapes in that compartment, so they were getting swept.

I moved around the unsetting of the mark bits to make it a bit more logical. I also added some extra assertions about this stuff.

Feel free to comment, Brendan. It's a pretty simple fix, so I don't think it needs review. I want to try to get it in tomorrow morning if it's green on try.
Attachment #508621 - Attachment is obsolete: true
Gregor: recursive gc, stuck mark bit, help?

/be
diff of last two attachments seems to be missing stuff comment 61 discusses -- lack of hg qref?

/be
Here's the actual patch, sorry.
Attachment #509615 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/db8be4e3f373
Whiteboard: [hardblocker][has patch] → [hardblocker][fixed-in-tracemonkey]
Crossing fingers that it sticks this time.
Whiteboard: [hardblocker][fixed-in-tracemonkey] → [hardblocker][fixed-in-tracemonkey][has patch]
Comment on attachment 509800 [details] [diff] [review]
real fix for orange

>+    /* Mark cross-compartment pointers. */
>+    void markCrossCompartment(JSTracer *trc);

Nit: s/pointers/wrappers/.

> JSCompartment::mark(JSTracer *trc)
> {
>-    for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront())
>-        MarkValue(trc, e.front().key, "cross-compartment wrapper");
>+    if (IS_GC_MARKING_TRACER(trc)) {
>+        JSRuntime *rt = trc->context->runtime;
>+        if (rt->gcCurrentCompartment != NULL && rt->gcCurrentCompartment != this)

Belated nit: don't test ptr != NULL, just test ptr truthiness.

>+            return;
>+        
>+        if (marked)
>+            return;
>+        
>+        marked = true;
>+    }

Nits:

* trailing whitespace on blank lines, please trim elsewhere if present
* blank lines here seem one too many, could use one after the rt decl and lose the second one above

>@@ -2193,26 +2181,27 @@ SweepCompartments(JSContext *cx, JSGCInv
>     JSCompartment **write = read;
>     JS_ASSERT(rt->compartments.length() >= 1);
>     JS_ASSERT(*rt->compartments.begin() == rt->atomsCompartment);
> 
>     while (read < end) {
>         JSCompartment *compartment = *read++;

Nit for Gregor: canonical names have been winning in SpiderMonkey. Suggest we use comp here, not compartment, when the opportunity to rename this local arises.

Nit on that nit: the double-indirect JSCompartment **c variable is named as canonical char or stdio int-cuz-of-EOF variables are (ch is a backup name for such), whereas the canonical name would be compp. No big deal, just noting for future reference.

> 
>         /* Unmarked compartments containing marked objects don't get deleted, except LAST_CONTEXT GC is performed. */
>-        if ((!compartment->marked && compartment->arenaListsAreEmpty()) || gckind == GC_LAST_CONTEXT) {
>+        if ((!compartment->isMarked() && compartment->arenaListsAreEmpty())
>+            || gckind == GC_LAST_CONTEXT)
>+        {

Thanks, and nit: it is easier to read the wrapped condition, nice! But the comment's overlong. May as well wrap it into major-comment style.

>+    /*
>+     * We should not be depending on cx->compartment in the GC, so set it to
>+     * NULL to look for violations.
>+     */
>+    SwitchToCompartment(cx, (JSCompartment *)NULL);
>+    

Trailing whitespace, puce alert (or should it be teal alert? ;-).

rs=me on fixing these nits (the ones introduced by this patch), any time.

/be
Attachment #509800 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: