Closed Bug 785206 Opened 12 years ago Closed 12 years ago

IonMonkey: WorkerThreadState::notify() is inefficient

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sstangl, Assigned: bhackett1024)

Details

Attachments

(1 file)

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().
If we do mean to notify all threads, renaming it notifyAll() to match common usage would be fine.
Attached patch patchSplinter Review
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)
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+
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.
You're right. We have some weird condvars.
https://hg.mozilla.org/projects/ionmonkey/rev/55e50f49712a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.