Incremental GC is false

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Alice0775 White, Assigned: nmatsakis)

Tracking

({regression})

26 Branch
mozilla26
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
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.
(Assignee)

Comment 3

4 years ago
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?
tracking-firefox26: --- → ?
(Assignee)

Comment 5

4 years ago
Created attachment 797811 [details] [diff] [review]
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).
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.
Keywords: regression
(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

4 years ago
(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.
(Assignee)

Comment 12

4 years ago
Created attachment 798800 [details] [diff] [review]
Bug910777.diff

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)
(Assignee)

Updated

4 years ago
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?
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 16

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
tracking-firefox26: ? → ---
You need to log in before you can comment on or make changes to this bug.