Last Comment Bug 785206 - IonMonkey: WorkerThreadState::notify() is inefficient
: IonMonkey: WorkerThreadState::notify() is inefficient
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-08-23 13:09 PDT by Sean Stangl [:sstangl]
Modified: 2012-08-23 16:48 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (1.57 KB, patch)
2012-08-23 16:12 PDT, Brian Hackett (:bhackett)
sstangl: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2012-08-23 13:09:10 PDT
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().
Comment 1 User image Sean Stangl [:sstangl] 2012-08-23 14:30:08 PDT
If we do mean to notify all threads, renaming it notifyAll() to match common usage would be fine.
Comment 2 User image Brian Hackett (:bhackett) 2012-08-23 16:12:47 PDT
Created attachment 654832 [details] [diff] [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.
Comment 3 User image Sean Stangl [:sstangl] 2012-08-23 16:21:19 PDT
Comment on attachment 654832 [details] [diff] [review]

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.
Comment 4 User image Brian Hackett (:bhackett) 2012-08-23 16:38:31 PDT
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.
Comment 5 User image Sean Stangl [:sstangl] 2012-08-23 16:45:18 PDT
You're right. We have some weird condvars.
Comment 6 User image Brian Hackett (:bhackett) 2012-08-23 16:48:47 PDT

Note You need to log in before you can comment on or make changes to this bug.