| Summary: | DFG should not always speculate that ConvertThis is operating on an object | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | ||||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
Implementing this reveals a different performance pathology: code that does this also tends to use Get/PutByVal by passing a string as the property and a non-array object as the base. The performance effect is not large overall. I recommend landing this fix so that we can go straight to rectifying the Get/PutByVal issue.
Benchmark report for SunSpider, V8, and Kraken.
VMs tested:
"TipOfTree" at /Volumes/Data/pizlo/secondary/OpenSource/WebKitBuild/Release/jsc
"ConvertThis" at /Volumes/Data/pizlo/OpenSource/WebKitBuild/Release/jsc
Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. 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.
TipOfTree ConvertThis
SunSpider:
3d-cube 7.9900+-0.0343 ? 8.0432+-0.1101 ?
3d-morph 8.1684+-0.1193 ? 8.2467+-0.1269 ?
3d-raytrace 8.2537+-0.0980 8.2504+-0.0936
access-binary-trees 1.8260+-0.0229 1.8133+-0.0202
access-fannkuch 7.8923+-0.1340 ^ 7.7265+-0.0190 ^ definitely 1.0215x faster
access-nbody 4.2669+-0.0469 ^ 4.2038+-0.0142 ^ definitely 1.0150x faster
access-nsieve 3.2290+-0.0107 ? 3.2324+-0.0184 ?
bitops-3bit-bits-in-byte 1.7477+-0.0194 1.7306+-0.0035
bitops-bits-in-byte 5.2038+-0.0443 5.1688+-0.0501
bitops-bitwise-and 3.5122+-0.0240 ? 3.5163+-0.0300 ?
bitops-nsieve-bits 5.6525+-0.0313 ? 5.6860+-0.0619 ?
controlflow-recursive 2.2668+-0.0257 ? 2.2872+-0.0296 ?
crypto-aes 6.8148+-0.0567 ! 7.0403+-0.1575 ! definitely 1.0331x slower
crypto-md5 3.0029+-0.0185 2.9893+-0.0331
crypto-sha1 2.7714+-0.0268 2.7382+-0.0216 might be 1.0121x faster
date-format-tofte 10.8459+-0.1957 10.6477+-0.1106 might be 1.0186x faster
date-format-xparb 9.9163+-0.1040 ! 10.3162+-0.0917 ! definitely 1.0403x slower
math-cordic 7.3648+-0.1474 7.3274+-0.0577
math-partial-sums 10.5181+-0.0532 10.4718+-0.0396
math-spectral-norm 3.2671+-0.0353 3.2413+-0.0055
regexp-dna 12.7210+-0.1166 ? 12.7908+-0.1646 ?
string-base64 5.9919+-0.1074 5.8544+-0.0799 might be 1.0235x faster
string-fasta 7.4867+-0.0251 ? 7.5445+-0.0475 ?
string-tagcloud 13.3702+-0.0521 ? 13.4123+-0.1107 ?
string-unpack-code 24.3851+-0.3199 ? 24.4155+-0.2681 ?
string-validate-input 6.8708+-0.1438 6.7404+-0.0596 might be 1.0193x faster
<arithmetic> * 7.1283+-0.0300 ? 7.1321+-0.0252 ?
<geometric> 5.8442+-0.0213 5.8372+-0.0202
<harmonic> 4.7561+-0.0165 4.7417+-0.0173
TipOfTree ConvertThis
V8:
crypto 80.7025+-0.2982 ? 80.7595+-0.3268 ?
deltablue 248.8816+-1.0934 ! 251.7477+-1.4598 ! definitely 1.0115x slower
earley-boyer 107.4001+-0.3481 107.3162+-0.6062
raytrace 65.4514+-0.1531 ? 65.5927+-0.2873 ?
regexp 126.6937+-0.3104 ^ 125.4931+-0.3218 ^ definitely 1.0096x faster
richards 221.1563+-0.9745 ! 224.7123+-1.1369 ! definitely 1.0161x slower
splay 120.7385+-1.2174 119.0992+-0.9204 might be 1.0138x faster
<arithmetic> 138.7177+-0.2699 ? 139.2458+-0.2973 ?
<geometric> * 125.0269+-0.1911 ? 125.1393+-0.2604 ?
<harmonic> 113.4545+-0.1434 113.3852+-0.2575
TipOfTree ConvertThis
Kraken:
ai-astar 796.6452+-1.9231 ! 816.1780+-12.1984 ! definitely 1.0245x slower
audio-beat-detection 212.4217+-1.0197 ? 213.4148+-1.9340 ?
audio-dft 274.9441+-4.3004 ? 276.2634+-4.6912 ?
audio-fft 137.2551+-0.3152 ? 137.4052+-0.3454 ?
audio-oscillator 273.6483+-1.8349 ? 276.7508+-3.8972 ? might be 1.0113x slower
imaging-darkroom 492.9157+-2.7823 490.8462+-3.6562
imaging-desaturate 245.3794+-0.3197 ? 246.3243+-0.8802 ?
imaging-gaussian-blur 644.8167+-1.4412 ? 645.0578+-1.2392 ?
json-parse-financial 63.5879+-0.3812 63.1272+-0.2714
json-stringify-tinderbox 84.1453+-0.2874 83.8305+-0.3708
stanford-crypto-aes 151.7926+-1.6979 150.0534+-1.3587 might be 1.0116x faster
stanford-crypto-ccm 116.7405+-0.5188 ? 117.3453+-0.8867 ?
stanford-crypto-pbkdf2 230.5106+-2.2750 229.7040+-2.2936
stanford-crypto-sha256-iterative 86.3382+-0.3972 86.2480+-0.3263
<arithmetic> * 272.2244+-0.5199 ? 273.7535+-1.0708 ?
<geometric> 206.5363+-0.4597 ? 206.8855+-0.6455 ?
<harmonic> 161.1289+-0.3882 160.9525+-0.4489
TipOfTree ConvertThis
All benchmarks:
<arithmetic> 105.6915+-0.1792 ! 106.2278+-0.3407 ! definitely 1.0051x slower
<geometric> 26.6709+-0.0662 26.6702+-0.0665
<harmonic> 8.3701+-0.0284 8.3452+-0.0299
TipOfTree ConvertThis
Geomean of preferred means:
<scaled-result> 62.3693+-0.1227 ? 62.5155+-0.1291 ?
Created attachment 110042 [details]
the patch
Comment on attachment 110042 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=110042&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1801 > + m_jit.andPtr(MacroAssembler::TrustedImm32(~TagBitUndefined), scratchGPR); Does this handle null and undefined? (In reply to comment #3) > (From update of attachment 110042 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110042&action=review > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1801 > > + m_jit.andPtr(MacroAssembler::TrustedImm32(~TagBitUndefined), scratchGPR); > > Does this handle null and undefined? That's the idea. After that and, null and undefined will look like null, and everything else will look non-null. Created attachment 110079 [details]
the patch with 32_64
|
There is code out there (and indeed some of it in our own SunSpider, as well as V8 and Kraken) that does: function foo(…) { …. this.bar …. } foo(…); So that ConvertThis sees undefined and needs to convert it to a global object. We should not just always fail speculation when this happens.