Bug 161268 – Make SpeculatedType a 64-bit integer

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

I plan to add two new entries to it, and we don't have enough space in the 32-bit int.

[Saam Barati]

Created attachment 287161 [details]
bench results

[Saam Barati]

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

[Saam Barati]

(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.

[Saam Barati]

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

[Saam Barati]

Created attachment 287164 [details]
patch

[Geoffrey Garen]

Comment on attachment 287164 [details]
patch

r=me

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[Saam Barati]

Basically, DFGNode must grow by 4 bytes on 32-bits to make this work.

[Saam Barati]

Created attachment 287183 [details]
patch

Up for review again since this changed quite a bit.

[Saam Barati]

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?

[WebKit Commit Bot]

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.

[Saam Barati]

Oh man, debug builds are broken. Let me fix that.

[Saam Barati]

(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?

[Benjamin Poulain]

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?

[Saam Barati]

(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.

[Saam Barati]

Created attachment 287239 [details]
patch for landing

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 287240 [details]
patch for landing

Trying to appease non Darwin ports.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 287240 [details]
patch for landing

Clearing flags on attachment: 287240

Committed r205107: <http://trac.webkit.org/changeset/205107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

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.