Closed Bug 1337209 Opened 3 years ago Closed 3 years ago

Add JS shell test mechanism for gray marking

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 2 obsolete files)

I wanted to write some shell tests for WeakMap-related gray stuff.
This seems to work. I will add sample tests before requesting review. But a basic check is:

print(`start ${finalizeCount()}`);
ensureGrayRoot().x = makeFinalizeObserver();
testMarkBitsAfterGC([ensureGrayRoot(), this, Object.create(null)]);
gc();
print(`no change ${finalizeCount()}`);
let bits = getMarkBitsAfterGC();
print(`gray root is ${bits[0]}`);
print(`global is ${bits[1]}`);
print(`dead object is ${bits[2]}`);
ensureGrayRoot().x = 7;
gc();
print(`counted ${finalizeCount()}`);
bits = getMarkBitsAfterGC();
print(`gray root is ${bits[0]}`);
print(`global is ${bits[1]}`);
print(`dead object is ${bits[2]}`);

(ignore the finalizeCount stuff.) This will print

start 0
no change 0
gray root is gray
global is black
dead object is dead
counted 1
gray root is gray
global is black
dead object is dead
Attachment #8834214 - Flags: feedback?(jcoppeard)
Comment on attachment 8834214 [details] [diff] [review]
Add JS shell test mechanism for gray marking

Review of attachment 8834214 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, especially because this will expose gray marking to the fuzzers.

::: js/src/shell/js.cpp
@@ +5604,5 @@
> +// For testing gray marking, ensureGrayRoot() will heap-allocate an address
> +// where we can store a JSObject*, and create a new object if one doesn't
> +// already exist. The address and a gray tracing function pointer will be
> +// registered with JS_SetGrayGCRootsTracer, and this address will also be
> +// stored in a PrivateValue in a <global>.__gray__.

Do we need to allocate a holder?  We should be able to have a JSObject* as part of the ShellContext that we always mark as a gray root.

It might make sense to have this object be an array (so it's a list of gray roots), but a plain object works too.

@@ +5612,5 @@
> +// black bit (assuming nothing is holding onto it.)
> +//
> +// The idea is that you can set up a whole graph of objects to be marked gray,
> +// hanging off of the object returned from ensureGrayRoot(). Then you GC to
> +// clear the black bits, and GC again to set the gray bits.

Do we need to GC twice?

@@ +5619,5 @@
> +// testMarkBitsAfterGC(), which takes an Array of objects (which will be marked
> +// black at the time they're passed in). After every call of gc() in the shell,
> +// their mark bits will be recorded and get be retrieved with
> +// getMarkBitsAfterGC(), in the form of an array of strings with each index
> +// corresponding to the original objects passed to testMarkBitsAfterGC().

This is a neat way to get around the barriers that mark everything black.

@@ +5661,5 @@
> +                                                         NonshrinkingGCObjectVector()));
> +    }
> +
> +    if (!args.get(0).isObject()) {
> +        JS_ReportErrorASCII(cx, "argument must be an Array of objects");

Do you need to check it's an ArrayObject too?

@@ +5713,5 @@
> +                color = "gray";
> +            else if (cell->isMarked(gc::BLACK))
> +                color = "black";
> +            else
> +                color = "unmarked";

I think this will return the current state of the mark bits for these objects rather than the state after the last GC.
Attachment #8834214 - Flags: feedback?(jcoppeard) → feedback+
Good points.

(In reply to Jon Coppeard (:jonco) from comment #2)
> ::: js/src/shell/js.cpp
> @@ +5604,5 @@
> > +// For testing gray marking, ensureGrayRoot() will heap-allocate an address
> > +// where we can store a JSObject*, and create a new object if one doesn't
> > +// already exist. The address and a gray tracing function pointer will be
> > +// registered with JS_SetGrayGCRootsTracer, and this address will also be
> > +// stored in a PrivateValue in a <global>.__gray__.
> 
> Do we need to allocate a holder?  We should be able to have a JSObject* as
> part of the ShellContext that we always mark as a gray root.

Yeah, my current patch does this, though I just noticed the comment still refers to it.

> It might make sense to have this object be an array (so it's a list of gray
> roots), but a plain object works too.

Hm, yeah. Though it's about the same if you do ensureGrayRoot.myroots = [].

> @@ +5612,5 @@
> > +// black bit (assuming nothing is holding onto it.)
> > +//
> > +// The idea is that you can set up a whole graph of objects to be marked gray,
> > +// hanging off of the object returned from ensureGrayRoot(). Then you GC to
> > +// clear the black bits, and GC again to set the gray bits.
> 
> Do we need to GC twice?

I thought so, but I guess it depends on the ordering of the GC operations. In practice, it seems like a single GC always works -- but when looking into that, I started running into cases where things are still in the nursery well after I thought they should be tenured, and I haven't worked through what's going on there yet.

> @@ +5619,5 @@
> > +// testMarkBitsAfterGC(), which takes an Array of objects (which will be marked
> > +// black at the time they're passed in). After every call of gc() in the shell,
> > +// their mark bits will be recorded and get be retrieved with

Oops. "get be".

> @@ +5661,5 @@
> > +                                                         NonshrinkingGCObjectVector()));
> > +    }
> > +
> > +    if (!args.get(0).isObject()) {
> > +        JS_ReportErrorASCII(cx, "argument must be an Array of objects");
> 
> Do you need to check it's an ArrayObject too?

I originally intended to, but then decided that it's an unnecessary restriction. In practice, it can be anything where JS_GetElement(i) succeeds for i=0..length-1. If you want to use a plain object that happens to have the right numeric properties, I'm fine with that. Besides, Array testing would be weird -- should I unwrap cross-compartment wrappers? I certainly can't unwrap the elements. (If you want to test the mark bits of objects in another compartment, you need to call testMarkBitsAfterGC in that compartment.)

> @@ +5713,5 @@
> > +                color = "gray";
> > +            else if (cell->isMarked(gc::BLACK))
> > +                color = "black";
> > +            else
> > +                color = "unmarked";
> 
> I think this will return the current state of the mark bits for these
> objects rather than the state after the last GC.

...uh, right. My original intention was to do this as part of the GC and record the results. But that was harder to do, and anyway it seemed like you might want to test read barriers with this too. Other than the name, do you see any problem with this testing the current mark bits?

Of course, this means I need to come up with new names... :-(
(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Other than the name, do you
> see any problem with this testing the current mark bits?

No, that seems fine.
Ok, this works for the tests I wanted to write now. I ran into a couple of wrinkles. You really need a separate gray root in each compartment, because otherwise you'll end up rooting a wrapper (and indirectly the wrapped things. And you want to test the mark bits of objects in different compartments, so I made it append to the current list instead of replacing it every time, so you can use multiple calls (one per compartment) to build up the full set of interest.

It would probably be nicer to name the things you're testing instead of using positions, but it felt like overkill.

Last, it turns out that I can't test gray map + black delegate => gray key, because actual WeakMap delegates are always key wrappers, so the black delegate endsup marking the key black. :-(
Attachment #8842266 - Flags: review?(jcoppeard)
Attachment #8834214 - Attachment is obsolete: true
Ok, this works for the tests I wanted to write now. I ran into a couple of wrinkles. You really need a separate gray root in each compartment, because otherwise you'll end up rooting a wrapper (and indirectly the wrapped things. And you want to test the mark bits of objects in different compartments, so I made it append to the current list instead of replacing it every time, so you can use multiple calls (one per compartment) to build up the full set of interest.

It would probably be nicer to name the things you're testing instead of using positions, but it felt like overkill.

Last, it turns out that I can't test gray map + black delegate => gray key, because actual WeakMap delegates are always key wrappers, so the black delegate endsup marking the key black. :-(
Attachment #8842267 - Flags: review?(jcoppeard)
Attachment #8842266 - Attachment is obsolete: true
Attachment #8842266 - Flags: review?(jcoppeard)
Comment on attachment 8842267 [details] [diff] [review]
Add JS shell test mechanism for gray marking

Review of attachment 8842267 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for adding this.  r=me with comments addressed.

::: js/src/shell/js.cpp
@@ +292,5 @@
> +    using Base = GCVector<JSObject*, 0, SystemAllocPolicy>;
> +
> +  public:
> +    explicit NonshrinkingGCObjectVector() : Base()
> +    {}

Is this constructor needed?

@@ +514,5 @@
> +{
> +    JSRuntime* rt = trc->runtime();
> +    for (ZonesIter zone(rt, SkipAtoms); !zone.done(); zone.next()) {
> +        for (CompartmentsInZoneIter comp(zone); !comp.done(); comp.next()) {
> +            auto priv = reinterpret_cast<ShellCompartmentPrivate*>(JS_GetCompartmentPrivate(comp.get()));

Can use static_cast here.

@@ +5801,5 @@
>  
> +// For testing gray marking, ensureGrayRoot() will heap-allocate an address
> +// where we can store a JSObject*, and create a new object if one doesn't
> +// already exist. The address and a gray tracing function pointer will be
> +// registered with JS_SetGrayGCRootsTracer, and this address will also be

nit: registration already happened when the worker was created.

@@ +5815,5 @@
> +//
> +// To test grayness, register the objects of interest with addMarkObservers(),
> +// which takes an Array of objects (which will be marked black at the time
> +// they're passed in). After every call of gc() in the shell, their mark bits
> +// will be recorded and get be retrieved with getMarks(), in the form of an

"get be"

@@ +5822,5 @@
> +
> +static ShellCompartmentPrivate*
> +EnsureShellCompartmentPrivate(JSContext* cx)
> +{
> +    auto priv = reinterpret_cast<ShellCompartmentPrivate*>(JS_GetCompartmentPrivate(cx->compartment()));

Can use static_cast.

@@ +5835,5 @@
> +EnsureGrayRoot(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    auto priv = EnsureShellCompartmentPrivate(cx);

Need to check return value here.

@@ +5846,5 @@
> +    return true;
> +}
> +
> +static MarkBitWatchers*
> +EnsureMarkBitWatchers(JSContext* cx)

Sometimes these are called watchers and sometimes observers.  We should pick one and rename the others.

@@ +5877,5 @@
> +        return false;
> +    }
> +
> +    // Nursery objects disallowed during major GC sweep. (?)
> +    cx->runtime()->gc.evictNursery();

What does the comment mean?  Is the minor GC necessary?

@@ +5891,5 @@
> +        if (!v.isObject()) {
> +            JS_ReportErrorASCII(cx, "argument must be an Array of objects");
> +            return false;
> +        }
> +        markWatchers->get().append(&v.toObject());

Check return value here.

@@ +5926,5 @@
> +                color = "gray";
> +            else if (cell->isMarked(gc::BLACK))
> +                color = "black";
> +            else
> +                color = "unmarked";

Interesting, I guess we can observe this during incremental GC.

@@ +5931,5 @@
> +        }
> +        JSString* s = JS_NewStringCopyZ(cx, color);
> +        if (!s)
> +            return false;
> +        NewbornArrayPush(cx, ret, StringValue(s));

Check return value here.

@@ +8590,5 @@
>  
>      DestructSharedArrayBufferMailbox();
>  
> +    // Must clear out some of sc's UniquePtrs before JS_DestroyContext.
> +    sc.reset();

It might be a good idea to set the context private to null here in case there are any callbacks that try to access the ShellContext after this point.

::: js/src/tests/js1_8_5/extensions/collect-gray.js
@@ +7,5 @@
> +  "Test gray marking";
> +
> +print(BUGNUMBER + ": " + summary);
> +
> +grayRoot().x = Object.create(null);

I think you changed grayRoot() to return an array.  Having it as an object does read nicely though so maybe object was a better idea after all.

@@ +44,5 @@
> +addMarkObservers([wrapper1, value1, wrapper2, value2]);
> +wrapper1 = wrapper2 = null;
> +value1 = value2 = null;
> +schedulegc(this);
> +//global.eval("schedulegc(this)");

Please remove this.

@@ +53,5 @@
> +assertEq(marks[2], 'gray', 'gray key 2');
> +assertEq(marks[3], 'gray', 'black map, gray key => gray value');
> +
> +// Blacken one of the keys
> +[wrapper1] = nondeterministicGetWeakMapKeys(wm).filter(k => k.name == "wrapper1");

Nice!  I guess you could get it out of the grayRoot too right?

@@ +76,5 @@
> +// wm is in a variable, so is black.
> +wm = new WeakMap();
> +
> +let key = Object.create(null);
> +global.grayRoot().delegate = key;

Might be worth a comment that this creates a wrapper.  I didn't notice that at first.

@@ +85,5 @@
> +wm.set(key, value);
> +grayRoot().value = value;
> +
> +// We are interested in the mark bits for the map, key, and value in both the
> +// main and other compartments. Note that the other-compartment key is the

This comment is slightly confusing because it implies that objects have different mark bits in each compartment.

@@ +107,5 @@
> +assertEq(map_mark, 'black', 'map in var => black');
> +assertEq(key_mark, 'gray', 'black map, gray delegate => gray key');
> +assertEq(value_mark, 'gray', 'black map, gray delegate/key => gray value');
> +
> +// Black map, black delegate => gray key

Should be black key, as the code tests.
Attachment #8842267 - Flags: review?(jcoppeard) → review+
Depends on: 1345307
(In reply to Jon Coppeard (:jonco) (Away until 20th March) from comment #7)
> @@ +5835,5 @@
> > +EnsureGrayRoot(JSContext* cx, unsigned argc, Value* vp)
> > +{
> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +
> > +    auto priv = EnsureShellCompartmentPrivate(cx);
> 
> Need to check return value here.

It's infallible.

> @@ +5846,5 @@
> > +    return true;
> > +}
> > +
> > +static MarkBitWatchers*
> > +EnsureMarkBitWatchers(JSContext* cx)
> 
> Sometimes these are called watchers and sometimes observers.  We should pick
> one and rename the others.

Fixed. Settled on observers.

> @@ +5877,5 @@
> > +        return false;
> > +    }
> > +
> > +    // Nursery objects disallowed during major GC sweep. (?)
> > +    cx->runtime()->gc.evictNursery();
> 
> What does the comment mean?  Is the minor GC necessary?

It is. I will rewrite the comment, now that I know what's going on, and perhaps fix it properly later.

WeakCache does not do any tracing. So if you put a nursery object into a WeakCache collection, it will not be moved during the next minor GC. Which means that at the moment, WeakCache is implicitly tenured-only.

I'd like to land with the evictNursery call in place. I'm attempting to write a patch that adds a trace() to WeakCache to mark the contents weakly, but it's a little weird because it means extending the GCPolicy<> stuff, and also you'd end up with the strange situation where you could do Rooted<WeakCache<T>> and the Rooted would update weak pointers but not root. Which would be strange, to have a Rooted that isn't rooted. So I may end up doing it a little differently, I'm not sure yet. The advantage is that it looks like I might be able to replace a few more one-off trace/sweep things if I have this in addition to the runtime-wide WeakCache that I already landed.

At any rate, the evictNursery here makes this patch landable while maintaining the hidden invariant that we currently have on WeakCaches. I sadly don't have an easy way to assert the invariant, but I'd rather work on removing it than asserting it.

> @@ +5926,5 @@
> > +                color = "gray";
> > +            else if (cell->isMarked(gc::BLACK))
> > +                color = "black";
> > +            else
> > +                color = "unmarked";
> 
> Interesting, I guess we can observe this during incremental GC.

Maybe? I haven't actually seen it.

> @@ +53,5 @@
> > +assertEq(marks[2], 'gray', 'gray key 2');
> > +assertEq(marks[3], 'gray', 'black map, gray key => gray value');
> > +
> > +// Blacken one of the keys
> > +[wrapper1] = nondeterministicGetWeakMapKeys(wm).filter(k => k.name == "wrapper1");
> 
> Nice!  I guess you could get it out of the grayRoot too right?

Yeah, this was really from an earlier iteration of the test. Replaced with grayRoot.

> @@ +85,5 @@
> > +wm.set(key, value);
> > +grayRoot().value = value;
> > +
> > +// We are interested in the mark bits for the map, key, and value in both the
> > +// main and other compartments. Note that the other-compartment key is the
> 
> This comment is slightly confusing because it implies that objects have
> different mark bits in each compartment.

Rewritten.

> @@ +107,5 @@
> > +assertEq(map_mark, 'black', 'map in var => black');
> > +assertEq(key_mark, 'gray', 'black map, gray delegate => gray key');
> > +assertEq(value_mark, 'gray', 'black map, gray delegate/key => gray value');
> > +
> > +// Black map, black delegate => gray key
> 
> Should be black key, as the code tests.

Doh!
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f01f3075fe
Add JS shell test mechanism for gray marking, r=jonco
https://hg.mozilla.org/mozilla-central/rev/95f01f3075fe
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1352507
Depends on: 1366925
You need to log in before you can comment on or make changes to this bug.