Closed Bug 910777 Opened 6 years ago Closed 6 years ago

Incremental GC is false

Categories

(Core :: JavaScript Engine, defect)

26 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: alice0775, Assigned: nmatsakis)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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
Missing JSCLASS_IMPLEMENTS_BARRIERS in a new js::Class?
We should add some test for this, that fails when IGC is disabled in a normal session.
This is probably the fault of bug 898347
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?
Attached patch Bug910777.diff (obsolete) — Splinter Review
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).
Attachment #797811 - Flags: review?(sphink)
Any reason this bug is not set to NEW or Assigned ?
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?
Assignee: general → nmatsakis
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: in-testsuite?
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.
(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...
(In reply to Tom Schuster [:evilpie] from comment #2)
Yes we should.  Filed bug 911182.
JSCLASS_IMPLEMENTS_BARRIERS is needed only if a class has a custom trace hook.
Attached patch Bug910777.diffSplinter Review
Based on billm's comment, removed the "implements barriers" annotations from those classes that do not define their own trace hooks.
Attachment #797811 - Attachment is obsolete: true
Attachment #797811 - Flags: review?(sphink)
Attachment #798800 - Flags: review?(sphink)
Blocks: 912108
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.)
Attachment #798800 - Flags: review?(sphink) → review+
checkin-needed? or more needs to happen here?
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.
Oh, I did not devise a test for this, and I am not 100% sure of how to do that.
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.
https://hg.mozilla.org/mozilla-central/rev/c8dffd55eeef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.