Bug 147473 – Convert Mac Watchdog implementation to use WTF::WorkQueue.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Mark Lam]

In this step, I'll just convert the Mac Watchdog implementation to use WorkQueue.  I'll make all platforms adopt the WorkQueue implementation in a subsequent patch.

[Mark Lam]

Created attachment 257878 [details]
the fix.

[Anders Carlsson]

Comment on attachment 257878 [details]
the fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=257878&action=review

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:40
> +    : m_queue(WorkQueue::create("jsc.watchdog.queue", WorkQueue::Type::Concurrent, WorkQueue::QOS::UserInteractive))

This shouldn't be concurrent.

[Mark Lam]

Created attachment 257932 [details]
fix 2: fixed to use a Serial WorkQueue

[Build Bot]

Comment on attachment 257932 [details]
fix 2: fixed to use a Serial WorkQueue

Attachment 257932 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3425

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html

[Build Bot]

Created attachment 257939 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Mark Lam]

(In reply to comment #4)
> Comment on attachment 257932 [details]
> fix 2: fixed to use a Serial WorkQueue
> 
> Attachment 257932 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/3425
> 
> New failing tests:
> platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-
> mainframe-slow-horizontal.html

The test failure is not related to this patch.

[Geoffrey Garen]

Comment on attachment 257932 [details]
fix 2: fixed to use a Serial WorkQueue

View in context: https://bugs.webkit.org/attachment.cgi?id=257932&action=review

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:37
>  
>  void Watchdog::initTimer()
>  {
> -    m_queue = 0;
> -    m_timer = 0;
>  }
>  
>  void Watchdog::destroyTimer()
>  {
> -    ASSERT(!m_timer);
> -    if (m_queue)
> -        dispatch_release(m_queue);
>  }

Why did you keep these empty functions?

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:40
> +    : m_queue(WorkQueue::create("jsc.watchdog.queue", WorkQueue::Type::Serial, WorkQueue::QOS::UserInteractive))

Since you've chosen a design that will always fire a timer, even if no watchdog violation has occurred, it's not good to give your timer a UserInteractive priority. Your timers will steal valuable cycles from the main application thread for no good reason. You want some lower priority. Maybe Default.

> Source/JavaScriptCore/runtime/WatchdogMac.cpp:50
> +uintptr_t Watchdog::Timer::nextTimerID()
> +{
> +    uintptr_t id = m_nextTimerID++;
> +    if (!id)
> +        id = m_nextTimerID++;
> +    return id;
> +}

Why does this code live in WatchdogMac? I thought the purpose of this change was to achieve some kind of cross-platform code sharing. Did you achieve that goal?

[Mark Lam]

Geoff and I discussed these offline.  I'll document some notes below so that anyone following can understand what's decided. 

(In reply to comment #7)
> > Source/JavaScriptCore/runtime/WatchdogMac.cpp:37
> >  void Watchdog::initTimer()
> >  ...
> >  void Watchdog::destroyTimer()
> >  ...
> 
> Why did you keep these empty functions?
> 
> > Source/JavaScriptCore/runtime/WatchdogMac.cpp:50
> > +uintptr_t Watchdog::Timer::nextTimerID()
> > ...
> 
> Why does this code live in WatchdogMac? I thought the purpose of this change
> was to achieve some kind of cross-platform code sharing. Did you achieve
> that goal?

I wanted to keep the change to use WorkQueue separate from the platform changes so as to make the patch easier review.  Geoff pointed out that it's not meaningful to make this change unless we can test on other platforms, which makes sense to me.  So, I'll go ahead and dup this bug against 147107, and just do both in one go. 


> > Source/JavaScriptCore/runtime/WatchdogMac.cpp:40
> > +    : m_queue(WorkQueue::create("jsc.watchdog.queue", WorkQueue::Type::Serial, WorkQueue::QOS::UserInteractive))
> 
> Since you've chosen a design that will always fire a timer, even if no
> watchdog violation has occurred, it's not good to give your timer a
> UserInteractive priority. Your timers will steal valuable cycles from the
> main application thread for no good reason. You want some lower priority.
> Maybe Default.

I had several thoughts on this:
1. The watchdog timer should fire rarely at the rate of the timeout.  However, in the patch that I posted, the implementation is bad i.e. the watchdog will fire at the rate at which well behaved code enters the VM, which can be fairly frequent because well-behave JS code is short, returns often, and re-enters the VM for async callbacks.  The implementation does not need to behave this way.  I will fix this in the patch that I upload to 147107.

2. When the watchdog timer expires, it should have a high enough priority to run even when badly behaved JS code is running in the UI thread and pegging the CPU.  However, in my testing, I find that using WorkQueue::QOS::Utility (which is a lower priority) is more than adequate to do the job.  So, I will switch to QOS::Utility instead.

I will now dup this bug to https://bugs.webkit.org/show_bug.cgi?id=147107, and continue the work there.

*** This bug has been marked as a duplicate of bug 147107 ***

[Geoffrey Garen]

> I had several thoughts on this:
> 1. The watchdog timer should fire rarely at the rate of the timeout. 
> However, in the patch that I posted, the implementation is bad i.e. the
> watchdog will fire at the rate at which well behaved code enters the VM,
> which can be fairly frequent because well-behave JS code is short, returns
> often, and re-enters the VM for async callbacks.  The implementation does
> not need to behave this way.  I will fix this in the patch that I upload to
> 147107.

In particular, I think we should store a "next desired fire time" instead of a UUID, and just wait for the last timer to fire before we schedule the next one, if and only if the next timer fires prior to the next desired fire time.