Closed
Bug 785206
Opened 12 years ago
Closed 12 years ago
IonMonkey: WorkerThreadState::notify() is inefficient
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sstangl, Assigned: bhackett1024)
Details
Attachments
(1 file)
1.57 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
If we do mean to notify all threads, renaming it notifyAll() to match common usage would be fine.
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
You're right. We have some weird condvars.
Assignee | ||
Comment 6•12 years ago
|
||
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.
Description
•