Title: [214794] releases/WebKitGTK/webkit-2.16
- Revision
- 214794
- Author
- carlo...@webkit.org
- Date
- 2017-04-03 08:43:58 -0700 (Mon, 03 Apr 2017)
Log Message
Merge r214323 - [JSC][DFG] Make addShouldSpeculateAnyInt more conservative to avoid regression caused by Double <-> Int52 conversions
https://bugs.webkit.org/show_bug.cgi?id=169998
Reviewed by Saam Barati.
JSTests:
* microbenchmarks/int52-back-and-forth.js: Added.
(shouldBe):
(num):
Source/_javascript_Core:
Double <-> Int52 and JSValue <-> Int52 conversions are not so cheap. Thus, Int52Rep is super carefully emitted.
We make addShouldSpeculateAnyInt more conservative to avoid regressions caused by the above conversions.
We select ArithAdd(Int52, Int52) only when this calculation is beneficial compared to added Int52Rep conversions.
This patch tighten the conditions of addShouldSpeculateAnyInt.
1. Honor DoubleConstant.
When executing imaging-darkroom, we have a thing like that,
132:< 2:loc36> DoubleConstant(Double|UseAsOther, AnyIntAsDouble, Double: 4607182418800017408, 1.000000, bc#114)
1320:< 1:loc38> Int52Rep(Check:Int32:@82, Int52|PureInt, Int32, Exits, bc#114)
1321:< 1:loc39> Int52Constant(Int52|PureInt, Boolint32Nonboolint32Int52, Double: 4607182418800017408, 1.000000, bc#114)
133:<!3:loc39> ArithSub(Int52Rep:@1320<Int52>, Int52Rep:@1321<Int52>, Int52|MustGen, Int52, CheckOverflow, Exits, bc#114)
The LHS of ArithSub says predicting Boolint32, and the rhs says AnyIntAsDouble. Thus we select ArithSub(Int52, Int52) instead
of ArithSub(Double, Double). However, it soon causes OSR exits. In imaging-darkroom, LHS's Int32 prediction will be broken.
While speculating Int32 in the above situation is reasonable approach since the given LHS says predicting Int32, this causes
severe performance regression.
Previously, we always select ArithSub(Double, Double). So accidentally, we do not encounter this misprediction issue.
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.
We can see the code like `lhs - 1.0` instead of `lhs - 1` in imaging-darkroom. It offers good information that lhs and
the resulting value would be double. Handling the above ArithSub in double seems more appropriate rather than handling
it in Int52.
So, in this patch, we honor DoubleConstant. If we find DoubleConstant on one operand, we give up selecting
Arith[Sub,Add](Int52, Int52). This change removes OSR exits occurr in imaging-darkroom right now.
2. Two Int52Rep(Double) conversions are not desirable.
We allow AnyInt ArithAdd only when the one operand of the binary operation should be speculated AnyInt. It is a bit conservative
decision. This is because Double to Int52 conversion is not so cheap. Frequent back-and-forth conversions between Double and Int52
rather hurt the performance. If the one operand of the operation is already Int52, the cost for constructing ArithAdd becomes
cheap since only one Double to Int52 conversion could be required.
This recovers some regression in assorted tests while keeping kraken crypto improvements.
3. Avoid frequent Int52 to JSValue conversions.
Int52 to JSValue conversion is not so cheap. Thus, we would like to avoid such situations. So, in this patch, we allow
Arith(Int52, Int52) with AnyIntAsDouble operand only when the node is used as number. By doing so, we avoid the case like,
converting Int52, performing ArithAdd, and soon converting back to JSValue.
The above 3 changes recover the regression measured in microbenchmarks/int52-back-and-forth.js and assorted benchmarks.
And still it keeps kraken crypto improvements.
baseline patched
imaging-darkroom 201.112+-3.192 ^ 189.532+-2.883 ^ definitely 1.0611x faster
stanford-crypto-pbkdf2 103.953+-2.325 100.926+-2.396 might be 1.0300x faster
stanford-crypto-sha256-iterative 35.103+-1.071 ? 36.049+-1.143 ? might be 1.0270x slower
* dfg/DFGGraph.h:
(JSC::DFG::Graph::addShouldSpeculateAnyInt):
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.16/JSTests/ChangeLog (214793 => 214794)
--- releases/WebKitGTK/webkit-2.16/JSTests/ChangeLog 2017-04-03 13:57:23 UTC (rev 214793)
+++ releases/WebKitGTK/webkit-2.16/JSTests/ChangeLog 2017-04-03 15:43:58 UTC (rev 214794)
@@ -1,3 +1,14 @@
+2017-03-23 Yusuke Suzuki <utatane....@gmail.com>
+
+ [JSC][DFG] Make addShouldSpeculateAnyInt more conservative to avoid regression caused by Double <-> Int52 conversions
+ https://bugs.webkit.org/show_bug.cgi?id=169998
+
+ Reviewed by Saam Barati.
+
+ * microbenchmarks/int52-back-and-forth.js: Added.
+ (shouldBe):
+ (num):
+
2017-03-23 Mark Lam <mark....@apple.com>
Clients of JSArray::tryCreateForInitializationPrivate() should do their own null checks.
Added: releases/WebKitGTK/webkit-2.16/JSTests/microbenchmarks/int52-back-and-forth.js (0 => 214794)
--- releases/WebKitGTK/webkit-2.16/JSTests/microbenchmarks/int52-back-and-forth.js (rev 0)
+++ releases/WebKitGTK/webkit-2.16/JSTests/microbenchmarks/int52-back-and-forth.js 2017-04-03 15:43:58 UTC (rev 214794)
@@ -0,0 +1,28 @@
+function shouldBe(actual, expected)
+{
+ if (actual !== expected)
+ throw new Error('bad value: ' + actual);
+}
+
+var array = [];
+
+for (var i = 0; i < 100; ++i)
+ array.push(1024 * 1024 * 1024 * 1024 + i);
+for (var i = 0; i < 100; ++i)
+ array.push(-(1024 * 1024 * 1024 * 1024 + i));
+
+array.push(2251799813685248);
+array.push(0.5);
+
+function num()
+{
+ return 42;
+}
+noInline(num);
+
+for (var i = 0; i < 1e4; ++i) {
+ for (var index = 0; index < 100; ++index) {
+ array[index] = (array[index] + 1) + num();
+ }
+}
+
Modified: releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/ChangeLog (214793 => 214794)
--- releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/ChangeLog 2017-04-03 13:57:23 UTC (rev 214793)
+++ releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/ChangeLog 2017-04-03 15:43:58 UTC (rev 214794)
@@ -1,3 +1,66 @@
+2017-03-23 Yusuke Suzuki <utatane....@gmail.com>
+
+ [JSC][DFG] Make addShouldSpeculateAnyInt more conservative to avoid regression caused by Double <-> Int52 conversions
+ https://bugs.webkit.org/show_bug.cgi?id=169998
+
+ Reviewed by Saam Barati.
+
+ Double <-> Int52 and JSValue <-> Int52 conversions are not so cheap. Thus, Int52Rep is super carefully emitted.
+ We make addShouldSpeculateAnyInt more conservative to avoid regressions caused by the above conversions.
+ We select ArithAdd(Int52, Int52) only when this calculation is beneficial compared to added Int52Rep conversions.
+
+ This patch tighten the conditions of addShouldSpeculateAnyInt.
+
+ 1. Honor DoubleConstant.
+
+ When executing imaging-darkroom, we have a thing like that,
+
+ 132:< 2:loc36> DoubleConstant(Double|UseAsOther, AnyIntAsDouble, Double: 4607182418800017408, 1.000000, bc#114)
+ 1320:< 1:loc38> Int52Rep(Check:Int32:@82, Int52|PureInt, Int32, Exits, bc#114)
+ 1321:< 1:loc39> Int52Constant(Int52|PureInt, Boolint32Nonboolint32Int52, Double: 4607182418800017408, 1.000000, bc#114)
+ 133:<!3:loc39> ArithSub(Int52Rep:@1320<Int52>, Int52Rep:@1321<Int52>, Int52|MustGen, Int52, CheckOverflow, Exits, bc#114)
+
+ The LHS of ArithSub says predicting Boolint32, and the rhs says AnyIntAsDouble. Thus we select ArithSub(Int52, Int52) instead
+ of ArithSub(Double, Double). However, it soon causes OSR exits. In imaging-darkroom, LHS's Int32 prediction will be broken.
+ While speculating Int32 in the above situation is reasonable approach since the given LHS says predicting Int32, this causes
+ severe performance regression.
+
+ Previously, we always select ArithSub(Double, Double). So accidentally, we do not encounter this misprediction issue.
+
+ 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.
+ We can see the code like `lhs - 1.0` instead of `lhs - 1` in imaging-darkroom. It offers good information that lhs and
+ the resulting value would be double. Handling the above ArithSub in double seems more appropriate rather than handling
+ it in Int52.
+
+ So, in this patch, we honor DoubleConstant. If we find DoubleConstant on one operand, we give up selecting
+ Arith[Sub,Add](Int52, Int52). This change removes OSR exits occurr in imaging-darkroom right now.
+
+ 2. Two Int52Rep(Double) conversions are not desirable.
+
+ We allow AnyInt ArithAdd only when the one operand of the binary operation should be speculated AnyInt. It is a bit conservative
+ decision. This is because Double to Int52 conversion is not so cheap. Frequent back-and-forth conversions between Double and Int52
+ rather hurt the performance. If the one operand of the operation is already Int52, the cost for constructing ArithAdd becomes
+ cheap since only one Double to Int52 conversion could be required.
+ This recovers some regression in assorted tests while keeping kraken crypto improvements.
+
+ 3. Avoid frequent Int52 to JSValue conversions.
+
+ Int52 to JSValue conversion is not so cheap. Thus, we would like to avoid such situations. So, in this patch, we allow
+ Arith(Int52, Int52) with AnyIntAsDouble operand only when the node is used as number. By doing so, we avoid the case like,
+ converting Int52, performing ArithAdd, and soon converting back to JSValue.
+
+ The above 3 changes recover the regression measured in microbenchmarks/int52-back-and-forth.js and assorted benchmarks.
+ And still it keeps kraken crypto improvements.
+
+ baseline patched
+
+ imaging-darkroom 201.112+-3.192 ^ 189.532+-2.883 ^ definitely 1.0611x faster
+ stanford-crypto-pbkdf2 103.953+-2.325 100.926+-2.396 might be 1.0300x faster
+ stanford-crypto-sha256-iterative 35.103+-1.071 ? 36.049+-1.143 ? might be 1.0270x slower
+
+ * dfg/DFGGraph.h:
+ (JSC::DFG::Graph::addShouldSpeculateAnyInt):
+
2017-03-23 Mark Lam <mark....@apple.com>
Clients of JSArray::tryCreateForInitializationPrivate() should do their own null checks.
Modified: releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/dfg/DFGGraph.h (214793 => 214794)
--- releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/dfg/DFGGraph.h 2017-04-03 13:57:23 UTC (rev 214793)
+++ releases/WebKitGTK/webkit-2.16/Source/_javascript_Core/dfg/DFGGraph.h 2017-04-03 15:43:58 UTC (rev 214794)
@@ -293,13 +293,44 @@
Node* left = add->child1().node();
Node* right = add->child2().node();
- auto isAnyIntSpeculationForAdd = [](SpeculatedType value) {
- return !!value && (value & (SpecAnyInt | SpecAnyIntAsDouble)) == value;
+ if (hasExitSite(add, Int52Overflow))
+ return false;
+
+ if (Node::shouldSpeculateAnyInt(left, right))
+ return true;
+
+ auto shouldSpeculateAnyIntForAdd = [](Node* node) {
+ auto isAnyIntSpeculationForAdd = [](SpeculatedType value) {
+ return !!value && (value & (SpecAnyInt | SpecAnyIntAsDouble)) == value;
+ };
+
+ // 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).
+ // In that case, we should honor this decision: using it as integer is not appropriate.
+ if (node->op() == DoubleConstant)
+ return false;
+ return isAnyIntSpeculationForAdd(node->prediction());
};
- return isAnyIntSpeculationForAdd(left->prediction())
- && isAnyIntSpeculationForAdd(right->prediction())
- && !hasExitSite(add, Int52Overflow);
+ // Allow AnyInt ArithAdd only when the one side of the binary operation should be speculated AnyInt. It is a bit conservative
+ // decision. This is because Double to Int52 conversion is not so cheap. Frequent back-and-forth conversions between Double and Int52
+ // rather hurt the performance. If the one side of the operation is already Int52, the cost for constructing ArithAdd becomes
+ // cheap since only one Double to Int52 conversion could be required.
+ // This recovers some regression in assorted tests while keeping kraken crypto improvements.
+ if (!left->shouldSpeculateAnyInt() && !right->shouldSpeculateAnyInt())
+ return false;
+
+ auto usesAsNumbers = [](Node* node) {
+ NodeFlags flags = node->flags() & NodeBytecodeBackPropMask;
+ if (!flags)
+ return false;
+ return (flags & (NodeBytecodeUsesAsNumber | NodeBytecodeNeedsNegZero | NodeBytecodeUsesAsInt | NodeBytecodeUsesAsArrayIndex)) == flags;
+ };
+
+ // Wrapping Int52 to Value is also not so cheap. Thus, we allow Int52 addition only when the node is used as number.
+ if (!usesAsNumbers(add))
+ return false;
+
+ return shouldSpeculateAnyIntForAdd(left) && shouldSpeculateAnyIntForAdd(right);
}
bool binaryArithShouldSpeculateInt32(Node* node, PredictionPass pass)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
