Bug 161268 - Make SpeculatedType a 64-bit integer
Summary: Make SpeculatedType a 64-bit integer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on: 161308 161427
Blocks: 160989
  Show dependency treegraph
 
Reported: 2016-08-26 14:38 PDT by Saam Barati
Modified: 2016-08-30 23:14 PDT (History)
13 users (show)

See Also:


Attachments
bench results (81.02 KB, text/plain)
2016-08-26 15:27 PDT, Saam Barati
no flags Details
patch (2.66 KB, patch)
2016-08-26 15:49 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch (26.56 KB, patch)
2016-08-26 17:51 PDT, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (27.67 KB, patch)
2016-08-28 14:55 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (28.05 KB, patch)
2016-08-28 15:22 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-08-26 14:38:49 PDT
I plan to add two new entries to it, and we don't have enough space in the 32-bit int.
Comment 1 Saam Barati 2016-08-26 15:27:29 PDT
Created attachment 287161 [details]
bench results
Comment 2 Saam Barati 2016-08-26 15:29:16 PDT
I ran sun spider for more iterations. Seems neutral:
VMs tested:
"og" at /Volumes/Data/WK/a/OpenSource/WebKitBuild/Release/jsc (r205031)
"change" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r205046)

Collected 20 samples per benchmark/VM, with 20 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times
with 95% confidence intervals in milliseconds.

                                      og                      change                                      

3d-cube                         4.4742+-0.1129     ?      4.8421+-0.5106        ? might be 1.0822x slower
3d-morph                        4.8733+-0.1396     ?      4.9183+-0.1489        ?
3d-raytrace                     4.7089+-0.1347     ?      4.8762+-0.2436        ? might be 1.0355x slower
access-binary-trees             2.0261+-0.2069     ?      2.0385+-0.2315        ?
access-fannkuch                 5.4078+-0.2145     ^      4.9936+-0.1585        ^ definitely 1.0829x faster
access-nbody                    2.3995+-0.2520            2.2865+-0.1149          might be 1.0494x faster
access-nsieve                   3.1811+-0.3418     ?      3.1835+-0.1785        ?
bitops-3bit-bits-in-byte        1.0306+-0.0917            1.0004+-0.0270          might be 1.0301x faster
bitops-bits-in-byte             2.4764+-0.1477     ?      2.5496+-0.1401        ? might be 1.0296x slower
bitops-bitwise-and              1.9476+-0.1216            1.9002+-0.0419          might be 1.0249x faster
bitops-nsieve-bits              2.9961+-0.1524     ?      3.0250+-0.1350        ?
controlflow-recursive           2.3993+-0.3184            2.1664+-0.0488          might be 1.1075x faster
crypto-aes                      4.1460+-0.1466     ?      4.5112+-0.3820        ? might be 1.0881x slower
crypto-md5                      2.6766+-0.2089            2.6467+-0.1506          might be 1.0113x faster
crypto-sha1                     2.5983+-0.0933            2.5894+-0.0765        
date-format-tofte               6.5434+-0.3718            6.2648+-0.1982          might be 1.0445x faster
date-format-xparb               4.6716+-0.5373            4.4075+-0.0530          might be 1.0599x faster
math-cordic                     2.6840+-0.1437     ?      2.7348+-0.2634        ? might be 1.0189x slower
math-partial-sums               3.8651+-0.1442            3.8397+-0.0633        
math-spectral-norm              1.9297+-0.0868            1.8879+-0.1242          might be 1.0221x faster
regexp-dna                      6.2257+-0.1885            6.0042+-0.0970          might be 1.0369x faster
string-base64                   4.3364+-0.5977            3.8230+-0.1789          might be 1.1343x faster
string-fasta                    5.2875+-0.1719     ?      5.4146+-0.2049        ? might be 1.0240x slower
string-tagcloud                 8.4890+-0.7232            7.9107+-0.2256          might be 1.0731x faster
string-unpack-code             17.6484+-0.4734           17.3723+-0.3443          might be 1.0159x faster
string-validate-input           3.8912+-0.1658     ?      3.9788+-0.2443        ? might be 1.0225x slower

<arithmetic>                    4.3428+-0.0622            4.2756+-0.0469          might be 1.0157x faster
Comment 3 Saam Barati 2016-08-26 15:29:55 PDT
(In reply to comment #1)
> Created attachment 287161 [details]
> bench results

Note that nothing changed in JSC between r205031 and r205046.
Also, octane/zlib is crashing for reasons unknown to me. I'm
filing a bug.
Comment 4 Saam Barati 2016-08-26 15:44:45 PDT
Ok, I was running a stale octane.
It wasn't crashing. It had a syntax error.
Here is a new run of octane.

Collected 4 samples per benchmark/VM, with 4 VM invocations per benchmark. Emitted a call to gc()
between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the
jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution
times with 95% confidence intervals in milliseconds.

                               og                      change                                      

encrypt                 0.14933+-0.00151    ?     0.14993+-0.00175       ?
decrypt                 2.50409+-0.01647    ?     2.52090+-0.02592       ?
deltablue      x2       0.12672+-0.00766          0.12368+-0.00276         might be 1.0246x faster
earley                  0.27113+-0.00582          0.27080+-0.00154       
boyer                   4.64829+-0.31196          4.48091+-0.27852         might be 1.0374x faster
navier-stokes  x2       4.63976+-0.06540    ?     4.64351+-0.08296       ?
raytrace       x2       0.75674+-0.00699          0.74796+-0.00414         might be 1.0117x faster
richards       x2       0.07671+-0.00295          0.07664+-0.00293       
splay          x2       0.32629+-0.00423    ?     0.32771+-0.00363       ?
regexp         x2      15.82487+-0.47393    ?    16.07022+-0.61532       ? might be 1.0155x slower
pdfjs          x2      35.63338+-1.05705    ?    36.21936+-0.34584       ? might be 1.0164x slower
mandreel       x2      39.41680+-0.41283    ?    39.71157+-0.30214       ?
gbemu          x2      27.84621+-0.47470    ?    27.90936+-0.49965       ?
closure                 0.46175+-0.00772    ?     0.46655+-0.00507       ? might be 1.0104x slower
jquery                  6.28443+-0.11179    ?     6.36670+-0.15409       ? might be 1.0131x slower
box2d          x2       8.59893+-0.11928    ?     8.67511+-0.04550       ?
zlib           x2     331.46051+-21.39808   ?   335.75600+-19.48167      ? might be 1.0130x slower
typescript     x2     576.16992+-14.91424       573.33197+-15.50176      

<geometric>             4.72847+-0.02567    ?     4.73646+-0.03637       ? might be 1.0017x slower
Comment 5 Saam Barati 2016-08-26 15:49:58 PDT
Created attachment 287164 [details]
patch
Comment 6 Geoffrey Garen 2016-08-26 15:51:41 PDT
Comment on attachment 287164 [details]
patch

r=me
Comment 7 WebKit Commit Bot 2016-08-26 15:53:00 PDT
Attachment 287164 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:44:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Saam Barati 2016-08-26 17:12:00 PDT
This has a nice tail of things that need to be changed for 32-bit.
Doing that now, and I'll upload a new patch for review.
Comment 9 Saam Barati 2016-08-26 17:48:17 PDT
Basically, DFGNode must grow by 4 bytes on 32-bits to make this work.
Comment 10 Saam Barati 2016-08-26 17:51:32 PDT
Created attachment 287183 [details]
patch

Up for review again since this changed quite a bit.
Comment 11 Saam Barati 2016-08-26 17:52:09 PDT
Comment on attachment 287183 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGNode.h:2342
> +    struct OpInfoWrapper {

I wonder if I should just turn OpInfo into this Struct. Thoughts?
Comment 12 WebKit Commit Bot 2016-08-26 17:52:51 PDT
Attachment 287183 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2361:  uint32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2362:  int32_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2363:  uint64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:44:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 8 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Saam Barati 2016-08-26 17:55:19 PDT
Oh man, debug builds are broken. Let me fix that.
Comment 14 Saam Barati 2016-08-26 17:55:34 PDT
(In reply to comment #11)
> Comment on attachment 287183 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287183&action=review
> 
> > Source/JavaScriptCore/dfg/DFGNode.h:2342
> > +    struct OpInfoWrapper {
> 
> I wonder if I should just turn OpInfo into this Struct. Thoughts?

Fil, Ben, any thoughts on this?
Comment 15 Benjamin Poulain 2016-08-26 18:00:44 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=287183&action=review

> Source/JavaScriptCore/dfg/DFGNode.h:-559
> -        m_opInfo = bitwise_cast<intptr_t>(data);

The operator=(void*) does not work here?

> Source/JavaScriptCore/dfg/DFGNode.h:570
> +        m_opInfo = nullptr;
> +        m_opInfo2 = nullptr;

m_opInfo.clear()?
or 
    m_opInfo = OpInfoWrapper()
?

> Source/JavaScriptCore/dfg/DFGNode.h:2279
> +        return static_cast<BasicBlockLocation*>(m_opInfo.u.pointer);

What do you think of:
    m_opInfo->as<BasicBlockLocation*>()
instead of casts at the call site?

> Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h:130
> +    uint64_t m_info;

Why is this switched to 64bits?
Comment 16 Saam Barati 2016-08-26 18:05:43 PDT
(In reply to comment #15)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287183&action=review
> 
> > Source/JavaScriptCore/dfg/DFGNode.h:-559
> > -        m_opInfo = bitwise_cast<intptr_t>(data);
> 
> The operator=(void*) does not work here?
> 
> > Source/JavaScriptCore/dfg/DFGNode.h:570
> > +        m_opInfo = nullptr;
> > +        m_opInfo2 = nullptr;
> 
> m_opInfo.clear()?
> or 
>     m_opInfo = OpInfoWrapper()
> ?
I'll do m_opInfo = OpInfoWrapper()

> 
> > Source/JavaScriptCore/dfg/DFGNode.h:2279
> > +        return static_cast<BasicBlockLocation*>(m_opInfo.u.pointer);
> 
> What do you think of:
>     m_opInfo->as<BasicBlockLocation*>()
> instead of casts at the call site?
Yeah, I was thinking of something like that as I was writing
all those casts. I'll do that. What do you think about the
asPtr<> instead of as<>?
> 
> > Source/JavaScriptCore/dfg/DFGPromotedHeapLocation.h:130
> > +    uint64_t m_info;
> 
> Why is this switched to 64bits?
I guess I wasn't sure if it was being used as 64 bits (it's not), and just to
be consistent and prevent gotchas in the future, I wanted to make it 32-bits.
I guess I can keep it as 32-bit.
Comment 17 Saam Barati 2016-08-28 14:55:35 PDT
Created attachment 287239 [details]
patch for landing
Comment 18 WebKit Commit Bot 2016-08-28 14:56:58 PDT
Attachment 287239 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2392:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2397:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2402:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:44:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 8 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Saam Barati 2016-08-28 15:22:11 PDT
Created attachment 287240 [details]
patch for landing

Trying to appease non Darwin ports.
Comment 20 WebKit Commit Bot 2016-08-28 15:23:57 PDT
Attachment 287240 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2387:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2392:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNode.h:2397:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:44:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOpInfo.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 8 in 6 files


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

Clearing flags on attachment: 287240

Committed r205107: <http://trac.webkit.org/changeset/205107>
Comment 22 WebKit Commit Bot 2016-08-28 16:33:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Yusuke Suzuki 2016-08-30 22:15:55 PDT
GTK port fails with the debug assertion. I think the reason is the following.
Now the struct layout of the AbstractValue becomes changed.
Before this change, AbstractValue was dense. But now padding is inserted due to 64bit SpeculatedType.
While the other fileds are initialized with zeros correctly, this padding area is not ensured that it is zeros. Looking.