Summary: | Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::timedWait() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, aroben, kbr, sfalken | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Chris Rogers
2011-02-14 12:31:43 PST
Created attachment 82352 [details]
Patch
The bug I'm seeing without this fix is that tryLock() will always fail on Windows when another thread is waiting on a ThreadCondition. Here's the pattern: // Worker thread waiting to be signalled { MutexLocker locker(m_backgroundThreadLock); while (!m_moreInputBuffered && !m_wantsToExit) m_backgroundThreadCondition.wait(m_backgroundThreadLock); } . . . // Another thread has some work and tries to signal (using a tryLock()) // Not using a MutexLocker looks strange, but we use a tryLock() instead because this is run on the real-time // thread where it is a disaster for the lock to be contended (causes audio glitching). It's OK if we fail to // signal from time to time, since we'll get to it the next time we're called. We're called repeatedly // and frequently (around every 3ms). The background thread is processing well into the future and has a considerable amount of // leeway here... if (m_backgroundThreadLock.tryLock()) { // <----------------- this will alway FAIL currently on Windows m_moreInputBuffered = true; m_backgroundThreadCondition.signal(); m_backgroundThreadLock.unlock(); } Comment on attachment 82352 [details]
Patch
Seems fine to make this change, although the design seems surprising. Why is the boolean variable shared between threads needed at all? Can you just use wait/signal without a separate boolean variable?
I'm wondering if we can use "fast mutexes" on windows instead: <http://msdn.microsoft.com/en-us/library/ms810047.aspx#locks__topic7>. Thanks Alexey. I think you may be right about my use of the bool variable being redundant. I'll have to look closer at what I was thinking there and may be able to simplify that. But I'll land this fix first. I'm by no means a Windows programming expert so I'm not sure about the "fast mutexes"... (In reply to comment #4) > I'm wondering if we can use "fast mutexes" on windows instead: <http://msdn.microsoft.com/en-us/library/ms810047.aspx#locks__topic7>. Those look like they're for implementing device drivers... Committed r78597: <http://trac.webkit.org/changeset/78597> |