911182 - add test that incremental GC is enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Bug 910777 is an example of how it is easy to forget to add JSCLASS_IMPLEMENTS_BARRIERS and turn off incremental GC for the whole browser.  Seems like we should add a test for this.

Side question: it seems like generational GC would have the same issue that JSCLASS_IMPLEMENTS_BARRIERS is trying to solve (i.e., binary extensions adding a js::Class that don't implement barriers will cause corruption).  Does the lack of JSCLASS_IMPLEMENTS_BARRIERS also disable ggc?  Seems like, because of moving, the problem is much worse and JSCLASS_IMPLEMENTS_BARRIERS won't help much.

Comment 1

[Andrew McCreight [:mccr8]]

It looks like the BARRIERS kill switch is the only way to disable the |rt->gcIncrementalEnabled| flag, outside of an API function that is never called, so maybe we could just assert on runtime shutdown that the flag is true?  A more precise approach would add a new flag to track when the flag is forcibly disabled, and then assert on that.

I guess you also want to only assert when |rt->gcMode == JSGC_MODE_INCREMENTAL| or something, because you don't care if the kill switch has been tripped in runtimes, like workers, that aren't using IGC anyways.

Comment 2

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #0)
> Side question: it seems like generational GC would have the same issue that
> JSCLASS_IMPLEMENTS_BARRIERS is trying to solve (i.e., binary extensions
> adding a js::Class that don't implement barriers will cause corruption). 
> Does the lack of JSCLASS_IMPLEMENTS_BARRIERS also disable ggc?  Seems like,
> because of moving, the problem is much worse and JSCLASS_IMPLEMENTS_BARRIERS
> won't help much.

Good point! My understanding is that binary extensions need to be rebuilt against each version of the browser, so should bump into our handlified and typesafe marking APIs. Of course, we do still have a ways to go there for complete coverage, so that isn't a total panacea.

Disabling GGC for non-IMPLEMENTS_BARRIERS classes, at least for the near term, is an excellent idea to make porting easier when we flip the switch.

Comment 3

[Luke Wagner [:luke]]

Would our reasoning about how the typesafe markers should catch GGC binary extension hazards extend to IGC such that we could remove JSCLASS_IMPLEMENTS_BARRIERS?

Comment 4

[Terrence Cole [:terrence]]

Yes, I believe so. Currently the JS::Heap<T> type that these markers take only do post-barriering. We will be expanding this to do pre-barriers as well when we incrementalize browser marking. Once this is done, it should be relatively hard to have a live gcthing pointer on the heap that is not wrapped in an automatically barriered type, at least outside SpiderMonkey.

Comment 5

[Luke Wagner [:luke]]

Great.  Do we have typesafe marking now or is that coming soon?

Comment 6

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

Attachment: test-inc-gc

Bobby, I'm sticking this in xpconnect for lack of a better place. Is that okay?

Comment 7

[Andrew McCreight [:mccr8]]

Won't this fail to catch the case where we run this test before we an instance of the class with the bad custom trace hook?

Comment 8

[Andrew McCreight [:mccr8]]

"before we an" --> "before we trace an"

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 797945 [details] [diff] [review]
test-inc-gc

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

::: js/xpconnect/tests/chrome/test_incremental_gc.xul
@@ +20,5 @@
> +  <script type="application/javascript"><![CDATA[
> +     const Ci = Components.interfaces;
> +     Components.utils.import("resource://gre/modules/Services.jsm");
> +     let winEnumer = Services.ww.getWindowEnumerator();
> +     let enabled = winEnumer.getNext()

As noted on IRC, please dump the ww junk and just use |window| here.

Comment 10

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #5)
> Great.  Do we have typesafe marking now or is that coming soon?

The new interface are the JS_CallHeapFooTracer functions starting here:
http://dxr.allizom.org/mozilla-central/source/js/src/jsapi.h#l2128

The old JS_CallFooTracer functions are still present (just above) at the moment. Our intent is to remove these as we can and when they are all gone, do a bulk rename to remove "Heap" and use the old names with new types.

The other part, of course, is full handlification of the API. This should be quite easy now, just extremely boring and time-consuming.

Comment 11

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

(In reply to Andrew McCreight [:mccr8] from comment #7)
> Won't this fail to catch the case where we run this test before we an
> instance of the class with the bad custom trace hook?

Sorry, I didn't see comment 1 before. That does sound like a better way to do it. I'll try to put something together for that.

Comment 12

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

Attachment: test-inc-gc

This is a more direct approach of the sort that Andrew proposed.

Comment 13

[Andrew McCreight [:mccr8]]

Ah, good point, failing immediately is much more useful than waiting until shutdown. :)

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 801867 [details] [diff] [review]
test-inc-gc

Nice, thanks!

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1653866a2e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf1653866a2e