Closed Bug 1290589 Opened 4 years ago Closed 4 years ago

Make JSRuntime's exclusiveAccessOwner a js::Thread::Id instead of a PRThread*

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(2 files, 2 obsolete files)

Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
Attachment #8776148 - Flags: review?(terrence) → review+
Splitting this patch into two parts: the operator== consting and the exclusiveAccessOwner changes. The latter is causing some issues, which I'm still debugging. Landing the former in the meantime.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #2)
> Splitting this patch into two parts: the operator== consting and the
> exclusiveAccessOwner changes. The latter is causing some issues, which I'm
> still debugging. Landing the former in the meantime.

Pushed under the wrong bug number: https://bugzilla.mozilla.org/show_bug.cgi?id=1290317#c4

Ah well...
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61757c08a5f
Make JSRuntime's exclusiveAccessOwner a js::Thread::Id instead of a PRThread*; r=terrence
Backed out for asserting mIsSome in testParallelCompile.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85272f8ab75c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b61757c08a5fbe9df2a1af7e462650b8c5b556ba
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33515679&repo=mozilla-inbound

TEST-PASS | js/src/jit-test/tests/asm.js/testParallelCompile.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
TEST-PASS | js/src/jit-test/tests/asm.js/testParallelCompile.js | Success (code 0, args "--no-asmjs")
Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Exit code: -11
FAIL - asm.js/testParallelCompile.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/asm.js/testParallelCompile.js | Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261 (code -11, args "")
INFO exit-status     : -11
INFO timed-out       : False
INFO stderr         2> Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Exit code: -11
FAIL - asm.js/testParallelCompile.js
Flags: needinfo?(nfitzgerald)
Ok, so these assertions that keep failing are totally bogus. We're checking some state before locking that is not synchronized by any lock. In fact, the whole *CanLock() functions are pretty useless right now. We have checks against reentrancy and wrong thread unlocking in DEBUG builds via pthreads' ERRORCHECK mode and via the AutoLockWhatever& parameters that are thread through all these functions. It would be nice to have some dynamic ordering checks against inversion, but as it stands now, these assertions we do have are hurting way more than they are helping. I'm going to remove them.
Flags: needinfo?(nfitzgerald)
See also comment 6.

PTHREAD_MUTEX_ERRORCHECK gives us this error checking against reentrancy and
unlocking an unlocked lock or another thread's lock already, so it isn't
needed. This also makes the *CanLock assertions no-ops.

In the future, it would be nice to introduce ordering checks against
inversions. I plan to do this in a follow up.
Attachment #8779091 - Flags: review?(terrence)
Attachment #8776148 - Attachment is obsolete: true
Comment on attachment 8779091 [details] [diff] [review]
Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions

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

This looks correct. That said, I'd really like to land the patch that passes the |const AutoLockForExclusiveAccess&| to the functions you're stripping of assertions either first or concurrent with this patch.
Attachment #8779091 - Flags: review?(terrence) → review+
Moved a comment about why the exclusive access lock is taken in AutoTraceSession
to right above AutoTraceSession's AutoLockForExclusiveAccess member declaration.
Attachment #8779538 - Flags: review+
Attachment #8779091 - Attachment is obsolete: true
The only place that lost a proof of lock holding assertion (as opposed to
ordering assertion) was js::ExclusiveContext::setCompartment.
Attachment #8779539 - Flags: review?(terrence)
Comment on attachment 8779539 [details] [diff] [review]
Part 1: Thread AutoLockForExclusiveAccess params through compartment setting functions as proof of lock holding

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

Awesome!
Attachment #8779539 - Flags: review?(terrence) → review+
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ff90165cc9
Part 0: Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefc85446e21
Part 1: Thread AutoLockForExclusiveAccess params through compartment setting functions as proof of lock holding; r=terrence
https://hg.mozilla.org/mozilla-central/rev/f8ff90165cc9
https://hg.mozilla.org/mozilla-central/rev/eefc85446e21
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.