RESOLVED FIXED 161268
Make SpeculatedType a 64-bit integer
https://bugs.webkit.org/show_bug.cgi?id=161268
Summary Make SpeculatedType a 64-bit integer
Saam Barati
Reported 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.
Attachments
bench results (81.02 KB, text/plain)
2016-08-26 15:27 PDT, Saam Barati
no flags
patch (2.66 KB, patch)
2016-08-26 15:49 PDT, Saam Barati
ggaren: review+
patch (26.56 KB, patch)
2016-08-26 17:51 PDT, Saam Barati
fpizlo: review+
patch for landing (27.67 KB, patch)
2016-08-28 14:55 PDT, Saam Barati
no flags
patch for landing (28.05 KB, patch)
2016-08-28 15:22 PDT, Saam Barati
no flags
Saam Barati
Comment 1 2016-08-26 15:27:29 PDT
Created attachment 287161 [details] bench results
Saam Barati
Comment 2 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
Saam Barati
Comment 3 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.
Saam Barati
Comment 4 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
Saam Barati
Comment 5 2016-08-26 15:49:58 PDT
Geoffrey Garen
Comment 6 2016-08-26 15:51:41 PDT
Comment on attachment 287164 [details] patch r=me
WebKit Commit Bot
Comment 7 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.
Saam Barati
Comment 8 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.
Saam Barati
Comment 9 2016-08-26 17:48:17 PDT
Basically, DFGNode must grow by 4 bytes on 32-bits to make this work.
Saam Barati
Comment 10 2016-08-26 17:51:32 PDT
Created attachment 287183 [details] patch Up for review again since this changed quite a bit.
Saam Barati
Comment 11 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?
WebKit Commit Bot
Comment 12 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.
Saam Barati
Comment 13 2016-08-26 17:55:19 PDT
Oh man, debug builds are broken. Let me fix that.
Saam Barati
Comment 14 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?
Benjamin Poulain
Comment 15 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?
Saam Barati
Comment 16 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.
Saam Barati
Comment 17 2016-08-28 14:55:35 PDT
Created attachment 287239 [details] patch for landing
WebKit Commit Bot
Comment 18 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.
Saam Barati
Comment 19 2016-08-28 15:22:11 PDT
Created attachment 287240 [details] patch for landing Trying to appease non Darwin ports.
WebKit Commit Bot
Comment 20 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.
WebKit Commit Bot
Comment 21 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>
WebKit Commit Bot
Comment 22 2016-08-28 16:33:40 PDT
All reviewed patches have been landed. Closing bug.
Yusuke Suzuki
Comment 23 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.
Note You need to log in before you can comment on or make changes to this bug.