Bug 115134 - Polling based watchdog for the DFG
Summary: Polling based watchdog for the DFG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-04-24 15:31 PDT by Mark Lam
Modified: 2013-04-24 19:59 PDT (History)
6 users (show)

See Also:


Attachments
benchmark result: run 1: DFG enabled, watchdog enabled. (23.16 KB, text/plain)
2013-04-24 15:40 PDT, Mark Lam
no flags Details
benchmark result: run 2: DFG enabled, watchdog enabled. (23.13 KB, text/plain)
2013-04-24 15:41 PDT, Mark Lam
no flags Details
the patch. (8.19 KB, patch)
2013-04-24 15:45 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff
updated patch w/ Geoff's feedback addressed, and the API test updated. Will go ahead and land this version. (16.87 KB, patch)
2013-04-24 19:50 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-04-24 15:31:51 PDT
Getting the DFG in on the watchdog action.  The strategy is to emit a speculation check for the watchdog timer firing.  If we have a watchdog timer fire, we'll OSR exit and let the baseline JIT do the dirty work.  Since this is a rare and probably near terminal condition, the perf of using an OSR exit to handle the watchdog timer is not such a big issue.

Of greater interest is the perf when the watchdog is enabled but not firing (compared to watchdog disabled as a baseline).  Benchmark results show that overall hit (on all benchmarks combined) is between 1 - 2%.  Some individual benchmarks e.g. temporal-structure shows a big drop.  However, disassembly of the DFG generated code and profiling info shows that the only difference is that the loops are executing the extra polling checks for the watchdog timer.  Nothing else has changed.  Hence the perf difference is due to cache alignment or other random factors.

Patch coming soon.
Comment 1 Mark Lam 2013-04-24 15:32:09 PDT
ref: <rdar://problem/4115952>
Comment 2 Mark Lam 2013-04-24 15:40:50 PDT
Created attachment 199524 [details]
benchmark result: run 1: DFG enabled, watchdog enabled.
Comment 3 Mark Lam 2013-04-24 15:41:21 PDT
Created attachment 199525 [details]
benchmark result: run 2: DFG enabled, watchdog enabled.
Comment 4 Mark Lam 2013-04-24 15:45:55 PDT
Created attachment 199527 [details]
the patch.

To be clear, the watchdog's setTimeLimit() still needs to be called before scripts are executed (or more strictly speaking ... when no JS frames are on the JS stack) in order for the script code to have the watchdog timer checks emitted and thereby be able to time out.
Comment 5 WebKit Commit Bot 2013-04-24 15:48:49 PDT
Attachment 199527 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/ExitKind.h', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/runtime/Watchdog.cpp']" exit_code: 1
Source/JavaScriptCore/dfg/DFGNodeType.h:269:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGNodeType.h:270:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Geoffrey Garen 2013-04-24 15:56:43 PDT
Comment on attachment 199527 [details]
the patch.

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

r=me

> Source/JavaScriptCore/runtime/Watchdog.cpp:82
>          // And if we've previously compiled any functions, we need to deopt

"revert" is a better word than "deopt" here -- we're not necessarily forcing a less optimal form -- we're just throwing away the form we have right now, and letting the engine choose whatever form it chooses the next time the function executes.

> Source/JavaScriptCore/runtime/Watchdog.cpp:83
>          // them because they don't habe the needed polling checks yet.

Typo: "habe".
Comment 7 Geoffrey Garen 2013-04-24 15:57:01 PDT
Is our current API test sufficient to enter the DFG and exercise this code?
Comment 8 Mark Lam 2013-04-24 19:50:22 PDT
Created attachment 199609 [details]
updated patch w/ Geoff's feedback addressed, and the API test updated.  Will go ahead and land this version.
Comment 9 Mark Lam 2013-04-24 19:59:09 PDT
Landed in r149089: <http://trac.webkit.org/changeset/149089>.