Closed Bug 1395366 Opened 7 years ago Closed 7 years ago

Intermittent js/src/jit-test/tests/modules/missing-export-offthread.js | Timeout (code -9, args "--baseline-eager") [150.0 s]

Categories

(Core :: JavaScript Engine, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jonco)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:product])

Attachments

(1 file)

there are 30 failures documented for this failure in the last 2 days:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1395366

these are linux64/debug spidermonkey-sm-compacting/debug jobs.  In fact we rarely run this and it is about 90% failing (effectively perma fail)

:naveed, I see you are the triage owner for this component, is this something you can find the right person to look at this?
Flags: needinfo?(nihsanullah)
Whiteboard: [stockwell needswork]
I was curious if one of our HelperThreads patches caused this so I bisected, but it looks like, quite surprisingly:

The first bad revision is:
changeset:   377646:fce9e9b0be2b
user:        Masatoshi Kimura <VYV03354@nifty.ne.jp>
date:        Fri Aug 25 22:29:07 2017 +0900
summary:     Bug 1098412 - Remove the legacy Iterator constructor. r=luke

I'll see if I can't see what it is real quick.
Blocks: 1098412
I think bug 1098412 is not at fault, it just tickled GC order in a way that exposes a latent bug:

The test calls StartOffThreadParseModule() however, because a GC is in process, the ModuleParseTask is enqueued in the  parseWaitingOnGC list.  When the current GC finishes, EnqueuePendingParseTasksAfterGC() is called, which goes through all the tasks in parseWaitingOnGC, but task->parseGlobal->zone()->wasGCStarted() is true so the ModuleParseTask stays in the parseWaitingOnGC list and never gets dispatched.  Finally, when FinishOffThreadModule() is called, OffThreadParsingMuustWaitForGC() returns false (despite the GC still being started), so there is no gc::FinishGC(cx) call and so it waits forever.

Seems like OffThreadParsingMustWaitForGC() (which just returns rt->activeGCInAtomsZone()) is too narrow.  Unconditionally calling gc::Finish(cx) in FinishOffThreadModule() fixes the hang but probably isn't the right fix.  Jon, do you know what is?
Flags: needinfo?(nihsanullah) → needinfo?(jcoppeard)
Assignee: nobody → jcoppeard
Blocks: 1322648
No longer blocks: 1098412
Flags: needinfo?(jcoppeard)
First of all this reverts the change I made in bug 1322648 to not enqueue parse tasks if their zone is being collected which is what's causing problems here.

The main change is to prevent zones indented for use by helper threads from being collected before that use has started.  This is accomplished by adding more state in the zone group.  There's some fiddling necessary to ensure that the state is reset correctly on error so that we can always clean up these zones correctly.
Attachment #8904207 - Flags: review?(sphink)
Comment on attachment 8904207 [details] [diff] [review]
bug1395366-helper-thread-group

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

Much nicer. What do you think of changing createdForHelperThread to ownedByHelperThread? It's a little weird that createdForHelperThread can change over a lifetime. And I *think* it would be parallel to ownedByCurrentThread, more or less?

Hm. Except it's *not* the same, because ownedByHelperThread would be set to true before it is done being used by the main thread, which seems wrong. reservedForHelperThread would be weird after it becomes Active.

You know, never mind. createdForHelperThread is fine.

::: js/src/gc/ZoneGroup.h
@@ +72,5 @@
> +  public:
> +    // Whether a zone in this group was created for use by a helper thread.
> +    bool createdForHelperThread() const {
> +        return helperThreadUse == HelperThreadUse::Pending ||
> +               helperThreadUse == HelperThreadUse::Active;

Arguable, but it feels like it would be very slightly simpler to understand if this were helperThreadUse != HelperThreadUse::None.
Attachment #8904207 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80146e7ec85
Extend zone group's state to cover those intended for future use by helper threads and disallow GC of such groups r=sfink
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22a0a710fbf9
Fix bustage caused by missing explicit keyword r=me
Blocks: 1386534
Depends on: 1401141
Whiteboard: [stockwell needswork] → [stockwell fixed:product]
You need to log in before you can comment on or make changes to this bug.