910777 - Incremental GC is false

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/aebdc69b02e5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130829030201

about:support say "Incremental GC" is false in today's Nightly ID:20130829030201

Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/252ed8959523
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130828082219
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/10f121af2f56
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130828083019
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=252ed8959523&tochange=10f121af2f56

Comment 1

[Luke Wagner [:luke]]

Missing JSCLASS_IMPLEMENTS_BARRIERS in a new js::Class?

Comment 2

[Tom S [:evilpie]]

We should add some test for this, that fails when IGC is disabled in a normal session.

Comment 3

[Niko Matsakis [:nmatsakis]]

This is probably the fault of bug 898347

Comment 4

[Boris Zbarsky [:bzbarsky]]

Aha!  That would explain why bug 898347 triggered bug 823822 to become kinda-permaorange!  See bug 823822 comment 39.  This even came up in conversation yesterday, but I figured we would have known from perf tests if we'd actually disabled incremental GC... Shows what I know.  :(

That said, it looks like none of the DOM JSClasses have JSCLASS_IMPLEMENTS_BARRIERS set either.  Why is that not causing problems?

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Bug910777.diff

Add JSCLASS_IMPLEMENTS_BARRIERS to the various classes introduced by binary data. Are these flags needed for all classes, or only non-native ones? I also switched the calls to `setFixedSlot` to `initFixedSlot`, since the slots are not previously assigned at that point (except for the default value of "undefined", I guess). I believe we implement all the requirements for this flag correctly (since the connections between JS objects that we construct are all basically immutable, it's not so hard).

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

Any reason this bug is not set to NEW or Assigned ?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Because to most people, an attached patch is considered more important. But now I've set it to make you happy.

That said, can we please get a test for this?

Comment 8

[Andrew McCreight [:mccr8]]

I think the proper "test" would be something like, during shutdown, we assert that the IGC kill switch hasn't tripped during execution, because potentially anything could set it off.  I don't recall precisely how it is implemented.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> This even came up in
> conversation yesterday, but I figured we would have known from perf tests if
> we'd actually disabled incremental GC... Shows what I know.  :(

Unfortunately, our perf tests all focus on throughput rather than measuring mainthread pause time...

Comment 10

[Luke Wagner [:luke]]

(In reply to Tom Schuster [:evilpie] from comment #2)
Yes we should.  Filed bug 911182.

Comment 11

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

JSCLASS_IMPLEMENTS_BARRIERS is needed only if a class has a custom trace hook.

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug910777.diff

Based on billm's comment, removed the "implements barriers" annotations from those classes that do not define their own trace hooks.

Comment 13

[Steve Fink [:sfink] [:s:]]

Comment on attachment 798800 [details] [diff] [review]
Bug910777.diff

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

Seems fine from what I can tell (and the init() stuff is much better. We ought to have automated checks for that.)

Comment 14

[Dietrich Ayala (:dietrich)]

checkin-needed? or more needs to happen here?

Comment 15

[Niko Matsakis [:nmatsakis]]

No, I pushed a try run <https://tbpl.mozilla.org/?tree=Try&rev=b2bbbd2d8600> and was hoping to wait for the results, but given that the run is relatively straightforward and is annoying to Nightly users, I can push quicker.

Comment 16

[Niko Matsakis [:nmatsakis]]

Oh, I did not devise a test for this, and I am not 100% sure of how to do that.

Comment 17

[Andrew McCreight [:mccr8]]

Luke filed bug 911182 for a test, so don't worry about it.  Bill or I will write a test that should detect this, assuming that whatever class this is hangs around for at least one GC.

Comment 18

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8dffd55eeef

Comment 19

[Ed Morley [:emorley]]

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