Bug 65825 - DFG JIT does not track speculation decisions for global variables
Summary: DFG JIT does not track speculation decisions for global variables
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-07 02:48 PDT by Filip Pizlo
Modified: 2011-08-08 05:51 PDT (History)
2 users (show)

See Also:


Attachments
the patch (8.65 KB, patch)
2011-08-07 02:54 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix tabs) (9.73 KB, patch)
2011-08-07 03:19 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix more tabs) (9.74 KB, patch)
2011-08-07 03:23 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2011-08-07 02:48:35 PDT
The DFG JIT tracks speculation decisions for block-local temporaries (in the backend) and variables than span blocks (during bytecode parsing via local variable tracking).  But it does not do this for global variables.  Hence code that uses a global variable "as if" it were a local variable can experience awkward pathologies.  For example:

x=5
x/=2
x++

Where x is not declared as a 'var' will result in the first line storing the Int32 representation of 5 into x; the second line checking what type x has, realizing that it's an int, and converting it to a double in order to perform a division and then storing the result into x as a double; and the third line speculating that x is an integer and failing speculation.

The DFG JIT should track speculations for global variables, so that such code will at the very least not perform speculations that contravene other speculations performed by the same code.
Comment 1 Filip Pizlo 2011-08-07 02:54:29 PDT
Created attachment 103175 [details]
the patch
Comment 2 WebKit Review Bot 2011-08-07 02:56:32 PDT
Attachment 103175 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Last 3072 characters of output:
Source/JavaScriptCore/dfg/DFGGraph.h:204:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:205:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:206:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:207:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:208:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:209:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:214:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:216:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/dfg/DFGGraph.h:215:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:217:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:218:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:219:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:220:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:221:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:222:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:223:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:239:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:240:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:241:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:242:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:247:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:248:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:254:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:255:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:256:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:257:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:258:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:259:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:260:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:261:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/JavaScriptCore/dfg/DFGGraph.h:262:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 62 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2011-08-07 03:19:25 PDT
Created attachment 103176 [details]
the patch (fix tabs)
Comment 4 WebKit Review Bot 2011-08-07 03:21:45 PDT
Attachment 103176 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/dfg/DFGGraph.h:253:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Filip Pizlo 2011-08-07 03:23:02 PDT
Created attachment 103177 [details]
the patch (fix more tabs)
Comment 6 Sam Weinig 2011-08-07 22:05:20 PDT
Does this end up helping bitops-bitwise-and.js?
Comment 7 Filip Pizlo 2011-08-07 22:33:35 PDT
(In reply to comment #6)
> Does this end up helping bitops-bitwise-and.js?

Only ever so slightly.  Maybe 2%, and it's hardly statistically significant.  It's mainly just a win on controlflow-recursive and crypto.

All this patch really does is makes sure that we have consistent speculations for each global variable, whereas before we might have speculate differently each time we load from it.  So it only "wins" in the sense that it removes some ugly pathologies that we otherwise had, where we first turned something into a double and then later speculated that it was an integer.
Comment 8 Gavin Barraclough 2011-08-07 23:38:44 PDT
Comment on attachment 103177 [details]
the patch (fix more tabs)

Look great, nice overall win!
Comment 9 Filip Pizlo 2011-08-08 01:37:27 PDT
Comment on attachment 103177 [details]
the patch (fix more tabs)

All tests appear to be passing, so I'll let it land.
Comment 10 WebKit Review Bot 2011-08-08 05:51:14 PDT
Comment on attachment 103177 [details]
the patch (fix more tabs)

Clearing flags on attachment: 103177

Committed r92593: <http://trac.webkit.org/changeset/92593>
Comment 11 WebKit Review Bot 2011-08-08 05:51:18 PDT
All reviewed patches have been landed.  Closing bug.