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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d502f1d70df1

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61757c08a5f
Make JSRuntime's exclusiveAccessOwner a js::Thread::Id instead of a PRThread*; r=terrence

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions

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.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e69421db848

Comment 9

[Terrence Cole [:terrence]]

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.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions

Moved a comment about why the exclusive access lock is taken in AutoTraceSession
to right above AutoTraceSession's AutoLockForExclusiveAccess member declaration.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Thread AutoLockForExclusiveAccess params through compartment setting functions as proof of lock holding

The only place that lost a proof of lock holding assertion (as opposed to
ordering assertion) was js::ExclusiveContext::setCompartment.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push for this and others: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ba2ddf4876

Comment 13

[Terrence Cole [: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!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8ff90165cc9
https://hg.mozilla.org/mozilla-central/rev/eefc85446e21