Bug 156567

Summary: implement dynamic scope accesses in the DFG/FTL
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
WIP
none
patch
none
patch
ggaren: review-, buildbot: commit-queue-
Archive of layout-test-results from ews115 for mac-yosemite
none
patch
none
patch
none
patch
ggaren: review+
patch for landing
none
patch for landing
none
patch for landing none

Description Saam Barati 2016-04-13 23:24:13 PDT
...
Comment 1 Saam Barati 2016-04-13 23:39:39 PDT
Created attachment 276374 [details]
WIP

this might work. I haven't tested it thoroughly. Still needs FTL support.
Comment 2 Saam Barati 2016-04-13 23:43:42 PDT
Created attachment 276375 [details]
WIP
Comment 3 Saam Barati 2016-04-17 15:42:06 PDT
Created attachment 276601 [details]
patch
Comment 4 WebKit Commit Bot 2016-04-17 15:45:01 PDT
Attachment 276601 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:154:  The parameter name "scope" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7837:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:997:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:1001:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2016-04-17 19:02:42 PDT
Created attachment 276619 [details]
patch

fix 64bit rebase compilation
Comment 6 WebKit Commit Bot 2016-04-17 19:03:46 PDT
Attachment 276619 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:154:  The parameter name "scope" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7837:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:997:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:1001:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Build Bot 2016-04-17 20:21:01 PDT
Comment on attachment 276619 [details]
patch

Attachment 276619 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1178305

New failing tests:
js/dom/dfg-put-to-readonly-property.html
Comment 8 Build Bot 2016-04-17 20:21:07 PDT
Created attachment 276620 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Geoffrey Garen 2016-04-18 11:48:04 PDT
Comment on attachment 276619 [details]
patch

r- due to EWS failure
Comment 10 Saam Barati 2016-04-18 12:30:39 PDT
Created attachment 276656 [details]
patch

I think this patch should work now. There was a subtle bug where I flushed registers before asking a JSValueOperand for it's GPR
which confused the DFG register allocator into thinking a particular register still held a particular value.
I also fixed armv7 setupArgumentsWithExecState
Comment 11 Saam Barati 2016-04-18 12:32:37 PDT
Created attachment 276657 [details]
patch

fix a comment.
Comment 12 WebKit Commit Bot 2016-04-18 12:34:07 PDT
Attachment 276657 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:154:  The parameter name "scope" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:997:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:1001:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2016-04-18 12:36:24 PDT
Created attachment 276658 [details]
patch

remove commented out code.
Comment 14 WebKit Commit Bot 2016-04-18 12:39:04 PDT
Attachment 276658 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Geoffrey Garen 2016-04-18 12:52:45 PDT
Comment on attachment 276658 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:17
> +        There was a subtle bug where we used to never compile the var injection vartiant of the 

variant

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2655
> +    case UnresolvedProperty:
> +    case UnresolvedPropertyWithVarInjectionChecks:

I think we want to OSR exit on these two types of access because the baseline JIT can gather profiling data that resolves them. To be conservative, you could say "OSR exit on these types, but only if I haven't done so already." Our hope is that the baseline JIT will produce profiling on first access. But if it fails, there's no reason to try again.
Comment 16 Saam Barati 2016-04-18 13:51:46 PDT
Created attachment 276663 [details]
patch for landing

I've implemented Geoff's suggested heuristic.
Comment 17 Saam Barati 2016-04-18 14:25:23 PDT
Created attachment 276669 [details]
patch for landing

rebased
Comment 18 WebKit Commit Bot 2016-04-18 14:45:30 PDT
Attachment 276669 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Saam Barati 2016-04-18 16:40:18 PDT
Created attachment 276680 [details]
patch for landing

efl build fix.
Comment 20 WebKit Commit Bot 2016-04-18 16:42:19 PDT
Attachment 276680 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:69:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/GetPutInfo.h:71:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 WebKit Commit Bot 2016-04-18 18:38:16 PDT
Comment on attachment 276680 [details]
patch for landing

Clearing flags on attachment: 276680

Committed r199699: <http://trac.webkit.org/changeset/199699>
Comment 22 WebKit Commit Bot 2016-04-18 18:38:24 PDT
All reviewed patches have been landed.  Closing bug.