1290156 - Remove `lockOwner`s since they are now redundant

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

We do all this bookkeeping about who owns what lock, but that is all redundant
now that we use `PTHREAD_MUTEX_ERRORCHECK` with `js::Mutex` in `DEBUG` builds.

Comment 1

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

Attachment: Remove the GCRuntime DEBUG-only lockOwner field

The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.

Comment 2

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

Attachment: Remove the DEBUG-only lockOwner member from GlobalHelperThreadState

The checks that the `lockOwner` member was used for are now redundant given that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.

In methods where we asserted `isLocked()`, we now pass in an
`AutoLockHelperThreadState` reference for compile-time proof of lock
holding. This is mostly a mechanical change.

Comment 3

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

Attachment: Remove the DEBUG-only lockOwner member from vixl::Simulator

The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.

Comment 4

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

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

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8775620 [details] [diff] [review]
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState

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

::: js/src/vm/Runtime.cpp
@@ +908,3 @@
>          MOZ_FALLTHROUGH;
>        case GCLock:
>          break;

It would be nice to find a way to assert ordering of these locks statically. Maybe AutoLockGC should take the HelperThreadLock in its constructor?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/573529c879ad
Remove the GCRuntime DEBUG-only lockOwner field; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc0a14de00a
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/41096329fbed
Remove the DEBUG-only lockOwner member from vixl::Simulator; r=terrence

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This or something else from the push appears to have turned 10.10 debug cpp tests permared: https://treeherder.mozilla.org/logviewer.html#?job_id=33028238&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8786fe9a1993

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a261bbbe735
Remove the GCRuntime DEBUG-only lockOwner field; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9bc531fdc75
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/3335ffe05307
Remove the DEBUG-only lockOwner member from vixl::Simulator; r=terrence

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a261bbbe735
https://hg.mozilla.org/mozilla-central/rev/a9bc531fdc75
https://hg.mozilla.org/mozilla-central/rev/3335ffe05307