Bug 54408 - Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::timedWait()
Summary: Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::time...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-14 12:31 PST by Chris Rogers
Modified: 2011-02-15 11:53 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.29 KB, patch)
2011-02-14 12:33 PST, Chris Rogers
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-02-14 12:31:43 PST
Fix Mutex::tryLock() on Windows to work properly with PlatformCondition::timedWait()
Comment 1 Chris Rogers 2011-02-14 12:33:20 PST
Created attachment 82352 [details]
Patch
Comment 2 Chris Rogers 2011-02-14 12:39:34 PST
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 3 Alexey Proskuryakov 2011-02-14 12:59:45 PST
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?
Comment 4 Alexey Proskuryakov 2011-02-14 13:13:08 PST
I'm wondering if we can use "fast mutexes" on windows instead: <http://msdn.microsoft.com/en-us/library/ms810047.aspx#locks__topic7>.
Comment 5 Chris Rogers 2011-02-14 13:16:31 PST
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"...
Comment 6 Kenneth Russell 2011-02-14 14:26:00 PST
(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...
Comment 7 Chris Rogers 2011-02-15 11:53:16 PST
Committed r78597: <http://trac.webkit.org/changeset/78597>