Last Comment Bug 785206 - IonMonkey: WorkerThreadState::notify() is inefficient
: IonMonkey: WorkerThreadState::notify() is inefficient
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 Brian Hackett (:bhackett) 2012-08-23 16:12:47 PDT
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.
Comment 3 Sean Stangl [:sstangl] 2012-08-23 16:21:19 PDT
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.
Comment 4 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 Sean Stangl [:sstangl] 2012-08-23 16:45:18 PDT
You're right. We have some weird condvars.
Comment 6 Brian Hackett (:bhackett) 2012-08-23 16:48:47 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/55e50f49712a

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