Closed Bug 1290156 Opened 4 years ago Closed 4 years ago

Remove `lockOwner`s since they are now redundant

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

(3 files)

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.
The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.
Attachment #8775619 - Flags: review?(terrence)
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.
Attachment #8775620 - Flags: review?(terrence)
The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.
Attachment #8775621 - Flags: review?(terrence)
Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
Attachment #8775619 - Flags: review?(terrence) → review+
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?
Attachment #8775620 - Flags: review?(terrence) → review+
Attachment #8775621 - Flags: review?(terrence) → review+
Pushed by nfitzgerald@mozilla.com:
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
Flags: needinfo?(nfitzgerald)
Pushed by nfitzgerald@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4a261bbbe735
https://hg.mozilla.org/mozilla-central/rev/a9bc531fdc75
https://hg.mozilla.org/mozilla-central/rev/3335ffe05307
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1291349
You need to log in before you can comment on or make changes to this bug.