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?
tracking-firefox26: --- → ?
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).
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
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...
JSCLASS_IMPLEMENTS_BARRIERS is needed only if a class has a custom trace hook.
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.