WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 287164
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug