Changeset 214323 in webkit
- Timestamp:
- Mar 23, 2017 3:53:54 PM (7 years ago)
- Location:
- trunk
- Files:
-
- 1 added
- 3 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/JSTests/ChangeLog
r214313 r214323 1 2017-03-23 Yusuke Suzuki <utatane.tea@gmail.com> 2 3 [JSC][DFG] Make addShouldSpeculateAnyInt more conservative to avoid regression caused by Double <-> Int52 conversions 4 https://bugs.webkit.org/show_bug.cgi?id=169998 5 6 Reviewed by Saam Barati. 7 8 * microbenchmarks/int52-back-and-forth.js: Added. 9 (shouldBe): 10 (num): 11 1 12 2017-03-23 Mark Lam <mark.lam@apple.com> 2 13 -
trunk/Source/JavaScriptCore/ChangeLog
r214321 r214323 1 2017-03-23 Yusuke Suzuki <utatane.tea@gmail.com> 2 3 [JSC][DFG] Make addShouldSpeculateAnyInt more conservative to avoid regression caused by Double <-> Int52 conversions 4 https://bugs.webkit.org/show_bug.cgi?id=169998 5 6 Reviewed by Saam Barati. 7 8 Double <-> Int52 and JSValue <-> Int52 conversions are not so cheap. Thus, Int52Rep is super carefully emitted. 9 We make addShouldSpeculateAnyInt more conservative to avoid regressions caused by the above conversions. 10 We select ArithAdd(Int52, Int52) only when this calculation is beneficial compared to added Int52Rep conversions. 11 12 This patch tighten the conditions of addShouldSpeculateAnyInt. 13 14 1. Honor DoubleConstant. 15 16 When executing imaging-darkroom, we have a thing like that, 17 18 132:< 2:loc36> DoubleConstant(Double|UseAsOther, AnyIntAsDouble, Double: 4607182418800017408, 1.000000, bc#114) 19 1320:< 1:loc38> Int52Rep(Check:Int32:@82, Int52|PureInt, Int32, Exits, bc#114) 20 1321:< 1:loc39> Int52Constant(Int52|PureInt, Boolint32Nonboolint32Int52, Double: 4607182418800017408, 1.000000, bc#114) 21 133:<!3:loc39> ArithSub(Int52Rep:@1320<Int52>, Int52Rep:@1321<Int52>, Int52|MustGen, Int52, CheckOverflow, Exits, bc#114) 22 23 The LHS of ArithSub says predicting Boolint32, and the rhs says AnyIntAsDouble. Thus we select ArithSub(Int52, Int52) instead 24 of ArithSub(Double, Double). However, it soon causes OSR exits. In imaging-darkroom, LHS's Int32 prediction will be broken. 25 While speculating Int32 in the above situation is reasonable approach since the given LHS says predicting Int32, this causes 26 severe performance regression. 27 28 Previously, we always select ArithSub(Double, Double). So accidentally, we do not encounter this misprediction issue. 29 30 One thing can be found that we have DoubleConstant in the RHS. It means that we have `1.0` instead of `1` in the code. 31 We can see the code like `lhs - 1.0` instead of `lhs - 1` in imaging-darkroom. It offers good information that lhs and 32 the resulting value would be double. Handling the above ArithSub in double seems more appropriate rather than handling 33 it in Int52. 34 35 So, in this patch, we honor DoubleConstant. If we find DoubleConstant on one operand, we give up selecting 36 Arith[Sub,Add](Int52, Int52). This change removes OSR exits occurr in imaging-darkroom right now. 37 38 2. Two Int52Rep(Double) conversions are not desirable. 39 40 We allow AnyInt ArithAdd only when the one operand of the binary operation should be speculated AnyInt. It is a bit conservative 41 decision. This is because Double to Int52 conversion is not so cheap. Frequent back-and-forth conversions between Double and Int52 42 rather hurt the performance. If the one operand of the operation is already Int52, the cost for constructing ArithAdd becomes 43 cheap since only one Double to Int52 conversion could be required. 44 This recovers some regression in assorted tests while keeping kraken crypto improvements. 45 46 3. Avoid frequent Int52 to JSValue conversions. 47 48 Int52 to JSValue conversion is not so cheap. Thus, we would like to avoid such situations. So, in this patch, we allow 49 Arith(Int52, Int52) with AnyIntAsDouble operand only when the node is used as number. By doing so, we avoid the case like, 50 converting Int52, performing ArithAdd, and soon converting back to JSValue. 51 52 The above 3 changes recover the regression measured in microbenchmarks/int52-back-and-forth.js and assorted benchmarks. 53 And still it keeps kraken crypto improvements. 54 55 baseline patched 56 57 imaging-darkroom 201.112+-3.192 ^ 189.532+-2.883 ^ definitely 1.0611x faster 58 stanford-crypto-pbkdf2 103.953+-2.325 100.926+-2.396 might be 1.0300x faster 59 stanford-crypto-sha256-iterative 35.103+-1.071 ? 36.049+-1.143 ? might be 1.0270x slower 60 61 * dfg/DFGGraph.h: 62 (JSC::DFG::Graph::addShouldSpeculateAnyInt): 63 1 64 == Rolled over to ChangeLog-2017-03-23 == -
trunk/Source/JavaScriptCore/dfg/DFGGraph.h
r214296 r214323 294 294 Node* right = add->child2().node(); 295 295 296 auto isAnyIntSpeculationForAdd = [](SpeculatedType value) { 297 return !!value && (value & (SpecAnyInt | SpecAnyIntAsDouble)) == value; 296 if (hasExitSite(add, Int52Overflow)) 297 return false; 298 299 if (Node::shouldSpeculateAnyInt(left, right)) 300 return true; 301 302 auto shouldSpeculateAnyIntForAdd = [](Node* node) { 303 auto isAnyIntSpeculationForAdd = [](SpeculatedType value) { 304 return !!value && (value & (SpecAnyInt | SpecAnyIntAsDouble)) == value; 305 }; 306 307 // When DoubleConstant node appears, it means that users explicitly write a constant in their code with double form instead of integer form (1.0 instead of 1). 308 // In that case, we should honor this decision: using it as integer is not appropriate. 309 if (node->op() == DoubleConstant) 310 return false; 311 return isAnyIntSpeculationForAdd(node->prediction()); 298 312 }; 299 313 300 return isAnyIntSpeculationForAdd(left->prediction()) 301 && isAnyIntSpeculationForAdd(right->prediction()) 302 && !hasExitSite(add, Int52Overflow); 314 // Allow AnyInt ArithAdd only when the one side of the binary operation should be speculated AnyInt. It is a bit conservative 315 // decision. This is because Double to Int52 conversion is not so cheap. Frequent back-and-forth conversions between Double and Int52 316 // rather hurt the performance. If the one side of the operation is already Int52, the cost for constructing ArithAdd becomes 317 // cheap since only one Double to Int52 conversion could be required. 318 // This recovers some regression in assorted tests while keeping kraken crypto improvements. 319 if (!left->shouldSpeculateAnyInt() && !right->shouldSpeculateAnyInt()) 320 return false; 321 322 auto usesAsNumbers = [](Node* node) { 323 NodeFlags flags = node->flags() & NodeBytecodeBackPropMask; 324 if (!flags) 325 return false; 326 return (flags & (NodeBytecodeUsesAsNumber | NodeBytecodeNeedsNegZero | NodeBytecodeUsesAsInt | NodeBytecodeUsesAsArrayIndex)) == flags; 327 }; 328 329 // Wrapping Int52 to Value is also not so cheap. Thus, we allow Int52 addition only when the node is used as number. 330 if (!usesAsNumbers(add)) 331 return false; 332 333 return shouldSpeculateAnyIntForAdd(left) && shouldSpeculateAnyIntForAdd(right); 303 334 } 304 335
Note: See TracChangeset
for help on using the changeset viewer.