IonMonkey: WorkerThreadState::notify() is inefficient

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sstangl, Assigned: bhackett)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
WorkerThreadState::notify() calls PR_NotifyAllCondVar().

If there are N threads waiting on a condvar, it is only possible for 1 thread to lock the condvar at a time. PR_NotifyAllCondVar() causes EACH thread to individually become unblocked, lock the internal cvar mutex, attempt to lock the external mutex, generally fail, unlock the internal cvar mutex, and block again, except for the single thread that wins the race.

Instead, we should just call PR_NotifyCondVar().
(Reporter)

Comment 1

5 years ago
If we do mean to notify all threads, renaming it notifyAll() to match common usage would be fine.
(Assignee)

Comment 2

5 years ago
Created attachment 654832 [details] [diff] [review]
patch

Yeah, I think the PR_NotifyAllCondVar dates from this code's earlier incarnation in bug 767223, where many items would be added to the worklist at once and we wanted all threads to wake up.  This patch uses PR_NotifyCondVar when adding a single item to the worklist.
Assignee: general → bhackett1024
Attachment #654832 - Flags: review?(sstangl)
(Reporter)

Comment 3

5 years ago
Comment on attachment 654832 [details] [diff] [review]
patch

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

::: js/src/jsworkers.cpp
@@ +207,5 @@
> +
> +void
> +WorkerThreadState::notifyAll(CondVar which)
> +{
> +    JS_ASSERT(isLocked());

The isLocked() assertions in notify() and notifyAll() can be removed: it's not necessary to lock the externel mutex of a condvar to signal on it.
Attachment #654832 - Flags: review?(sstangl) → review+
(Assignee)

Comment 4

5 years ago
Actually, the docs are pretty clear that the lock needs to be held when notifying, presumably to avoid racing on the condvar's internal state between multiple calls to PR_Notify* or between PR_Notify* and PR_WaitCondVar.
(Reporter)

Comment 5

5 years ago
You're right. We have some weird condvars.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/55e50f49712a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.