Bug 170655

Summary: [JSC] Enable JSRunLoopTimer for JSCOnly and Windows
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cgarcia, clopez, cmarcelo, dbates, keith_miller, mark.lam, mcatanzaro, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch cgarcia: review+

Yusuke Suzuki
Reported 2017-04-09 08:54:26 PDT
[JSC] Enable JSRunLoopTimer for JSCOnly and Windows
Attachments
Patch (16.11 KB, patch)
2017-04-09 08:57 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (15.33 KB, patch)
2017-04-10 00:50 PDT, Yusuke Suzuki 		cgarcia: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2017-04-09 08:57:21 PDT
Created attachment 306622 [details] Patch
Carlos Garcia Campos
Comment 2 2017-04-09 23:29:04 PDT
Comment on attachment 306622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306622&action=review > Source/JavaScriptCore/jsc.cpp:3891 > WTF::initializeMainThread(); > JSC::initializeThreading(); > + RunLoop::initializeMainRunLoop(); Do we really need this? JS timers use RunLoop::current() not main(). If we need it, then we can remove WTF::initializeMainThread(); because initializeMainRunLoop already initializes the main thread. > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:190 > + m_timer.stop(); > + m_timer.startOneShot(s_decade); RunLoop::Timer::start() already calls stop(). > Source/JavaScriptCore/runtime/JSRunLoopTimer.h:85 > + static const Seconds s_decade; > + RunLoop::Timer<JSRunLoopTimer> m_timer; Yes, I think we could adopt this implementation at least for non cocoa based ports. I think when we added support for heap timer, RunLoop was still in WebCore.
Yusuke Suzuki
Comment 3 2017-04-10 00:46:07 PDT
Comment on attachment 306622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306622&action=review Thanks, I'll update the patch. >> Source/JavaScriptCore/jsc.cpp:3891 >> + RunLoop::initializeMainRunLoop(); > > Do we really need this? JS timers use RunLoop::current() not main(). If we need it, then we can remove WTF::initializeMainThread(); because initializeMainRunLoop already initializes the main thread. Right, it is not necessary. Dropped. >> Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:190 >> + m_timer.startOneShot(s_decade); > > RunLoop::Timer::start() already calls stop(). Nice, dropped. >> Source/JavaScriptCore/runtime/JSRunLoopTimer.h:85 >> + RunLoop::Timer<JSRunLoopTimer> m_timer; > > Yes, I think we could adopt this implementation at least for non cocoa based ports. I think when we added support for heap timer, RunLoop was still in WebCore. Yeah, non cocoa based ports, I'll change to use this RunLoop::Timer in the subsequent patch. This patch paves the way for that.
Yusuke Suzuki
Comment 4 2017-04-10 00:50:47 PDT
Created attachment 306667 [details] Patch
Yusuke Suzuki
Comment 5 2017-04-11 02:34:44 PDT
Committed r215223: <http://trac.webkit.org/changeset/215223>
Note You need to log in before you can comment on or make changes to this bug.