Bug 151972 - we should emit op_watchdog after op_enter
Summary: we should emit op_watchdog after op_enter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
: 147998 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-12-07 16:28 PST by Saam Barati
Modified: 2015-12-18 15:44 PST (History)
11 users (show)

See Also:


Attachments
patch (5.80 KB, patch)
2015-12-08 12:22 PST, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-12-07 16:28:54 PST
This will take care of loops that implemented using tail calls.
Comment 1 Saam Barati 2015-12-08 12:22:25 PST
Created attachment 266926 [details]
patch
Comment 2 WebKit Commit Bot 2015-12-08 12:23:37 PST
Attachment 266926 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:183:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:184:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2015-12-08 12:36:25 PST
Comment on attachment 266926 [details]
patch

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

r=me

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:200
> +                printf("PASS: %s script timed out as expected saamy.\n", tierOptions.tier);

typo: I think you didn't mean to have " saamy" in this line.

I also suggest adding a bit more details to this line to distinguish test from the one before e.g. "PASS: %s script with infinite tail calls timed out as expected.\n"

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:203
> +                    printf("FAIL: %s script did not time out as expected.\n", tierOptions.tier);

I suggest "FAIL: %s script with infinite tail calls did not time out as expected.\n".

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:205
> +                    printf("FAIL: %s script timeout callback was not called.\n", tierOptions.tier);

I suggest "FAIL: %s script with infinite tail calls' timeout callback was not called.\n".
Comment 4 Mark Lam 2015-12-08 12:37:55 PST
*** Bug 147998 has been marked as a duplicate of this bug. ***
Comment 5 Geoffrey Garen 2015-12-08 13:20:59 PST
Maybe it would be better to emit at op_tail_call instead. That way you don’t pay the cost for simple function calls.
Comment 6 Saam Barati 2015-12-09 10:23:20 PST
We've decided to do it at op_enter just to make life easier for the sampling profiler.
Comment 7 Saam Barati 2015-12-09 10:36:05 PST
landed in:
http://trac.webkit.org/changeset/193842
Comment 8 Saam Barati 2015-12-18 15:44:35 PST
forgot to mark this as fixed.