Closed Bug 1283229 Opened 4 years ago Closed 4 years ago

Thread `AutoLockHelperThreadState&` parameters through various `HelperThread`-related functions

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
50.2 - Jul 4
Tracking Status
firefox50 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 2 obsolete files)

This is a pre-requisite for moving the `HelperThread` infrastructure over from
using `PRLock` and `PRCondVar` to using `js::Mutex` and `js::ConditionVariable`.
Its needed because waiting on a `js::ConditionVariable` takes the `LockGuard`
for the condvar's mutex as a parameter, so we need to have that accessible. When
we do the conversion, `AutoLockHelperThreadState` will become derived from
`LockGuard` and specialized for the helper thread state lock, and so the guard
will therefore be accessible.
Also: this has the nice side effect of leveraging the type system to prove that
we have taken the lock at various points, although I don't remove the
now-mostly-redundant MOZ_ASSERT(HelperThreadState().isLocked()) checks in this
patch. It is still technically possible that someone snuck in an unlocked the
lock from under our feet (perhaps with AutoUnlockHelperThreadState) since we
don't have rust's borrow checker.
Attachment #8766464 - Flags: review?(terrence)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html]
FWIW: this is pretty much all mechanical.
Attachment #8766464 - Attachment is obsolete: true
Attachment #8766464 - Flags: review?(terrence)
s/MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM/MOZ_GUARD_OBJECT_NOTIFIER_PARAM/ for
AutoUnlockHelperThreadState which gained a non-guard object parameter and was
causing hazard analysis builds to fail in the try push.
Attachment #8766566 - Flags: review?(terrence)
Attachment #8766509 - Attachment is obsolete: true
Attachment #8766509 - Flags: review?(terrence)
Comment on attachment 8766566 [details] [diff] [review]
Thread `AutoLockHelperThreadState&` parameters through various `HelperThread`-related functions

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

Sorry for the churn I've requested. Should be limited to jsgc.cpp at least.

::: js/src/gc/GCRuntime.h
@@ +1199,5 @@
>      /*
>       * Concurrent sweep infrastructure.
>       */
> +    void startTask(AutoLockHelperThreadState& locked, GCParallelTask& task, gcstats::Phase phase);
> +    void joinTask(AutoLockHelperThreadState& locked, GCParallelTask& task, gcstats::Phase phase);

I'd like |locked| to be the last parameter here to match how we've used AutoLockGC everywhere else.

::: js/src/vm/HelperThreads.h
@@ +265,5 @@
>          switch (which) {
>            case CONSUMER: return consumerWakeup;
>            case PRODUCER: return producerWakeup;
>            case PAUSE: return pauseWakeup;
> +          default: MOZ_CRASH("Invalid CondVar in |whichWakeup|");

Thanks!
Attachment #8766566 - Flags: review?(terrence) → review+
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ad05d9cad4
Thread `AutoLockHelperThreadState&` parameters through various `HelperThread`-related functions; r=terrence
https://hg.mozilla.org/mozilla-central/rev/e5ad05d9cad4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: [devtools-html]
You need to log in before you can comment on or make changes to this bug.