Bug 24047 - Need to simplify nested if's in WorkerRunLoop::runInMode
Summary: Need to simplify nested if's in WorkerRunLoop::runInMode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-19 17:16 PST by David Levin
Modified: 2009-02-24 01:14 PST (History)
1 user (show)

See Also:


Attachments
Proposed fix. (9.43 KB, patch)
2009-02-20 07:56 PST, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (10.28 KB, patch)
2009-02-20 15:33 PST, David Levin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-02-19 17:16:55 PST
As part of this, it would be nice to also remove duplicate code in MessageQueue and add a method waitForMessageFilteredWithTimeout.
Comment 1 David Levin 2009-02-20 07:56:21 PST
Created attachment 27829 [details]
Proposed fix.
Comment 2 David Levin 2009-02-20 07:58:53 PST
This doesn't fire timers in nested loops but as I get to the point of testing that, this change should make it very easy to change that behavior as needed.
Comment 3 Dmitry Titov 2009-02-20 11:13:34 PST
As discussed on irc, the overflow check in ThreadingWin and ThreadingQt that protects from double overflow could be merged with the subsequent check that protects from int overlflow so there is only one check, something like:
// Overflow protection
if (absoluteTime > currentTime + static_cast<double>(INT_MAX) / 1000.0) {
    wait(mutex);
    return true;
}

ThreadingPthread and ThreadingGtk are fine as is since they don't put milliseconds in an int.
Comment 4 David Levin 2009-02-20 15:33:53 PST
Created attachment 27840 [details]
Proposed fix.

Addressed dimich's feedback.
Comment 5 David Levin 2009-02-20 15:35:41 PST
The changelog shows up in the middle of other changes.  I'll move it to the top before checking in.
Comment 6 Alexey Proskuryakov 2009-02-23 05:49:26 PST
Comment on attachment 27840 [details]
Proposed fix.

> +    inline MessageQueueWaitResult MessageQueue<DataType>::waitForMessageFilteredWithTimeout(DataType& result, Predicate& predicate, double absoluteTime)
>      {
> -        MutexLocker lock(m_mutex);
> +        bool allowTimeOut = absoluteTime != infiniteTime();

Inconsistent naming here: "timeout" is a word, so it should be allowTimeout or allowToTimeOut.

>          bool timedOut = false;

This one is fine.

> +        while (!m_killed  && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end())

There are two spaces before &&.

> +            timedOut = !m_condition.timedWait(m_mutex, absoluteTime) && allowTimeOut;

This looks suspicious. Can timedWait() return false for other reasons? If it can, is the problem limited to cases where allowTimeOut is false?

> +    // Time is too far in the future (and would overflow int)  - wait forever.
> +    if (absoluteTime - currentTime > static_cast<double>(INT_MAX) / 1000.0) {
> +        wait(mutex);
> +        return true;
> +    }
>  
> +    double intervalMilliseconds = (absoluteTime - currentTime) * 1000.0;

Isn't this subject to rounding errors? I don't see what guarantees that intervalMilliseconds won't overflow when converted to unsigned long.

> +    double absoluteTime = predicate.isDefaultMode() && m_sharedTimer->isActive() ? m_sharedTimer->fireTime() : MessageQueue<RefPtr<Task> >::infiniteTime();

I think that parentheses around the ?: predicate would help readability.

r=me, but please consider the comments.
Comment 7 David Levin 2009-02-24 01:14:52 PST
Commited in r41156.