Diff
Modified: trunk/JSTests/ChangeLog (261730 => 261731)
--- trunk/JSTests/ChangeLog 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/JSTests/ChangeLog 2020-05-15 06:55:08 UTC (rev 261731)
@@ -1,3 +1,13 @@
+2020-05-14 Saam Barati <[email protected]>
+
+ GetByVal and PutByVal runtime operations shouldn't fall off a performance cliff when the property is an integer boxed as a double
+ https://bugs.webkit.org/show_bug.cgi?id=211935
+
+ Reviewed by Yusuke Suzuki and Mark Lam.
+
+ * microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js: Added.
+ (test):
+
2020-05-14 Devin Rousso <[email protected]>
[ESNext] enable logical assignment operators by default
Added: trunk/JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js (0 => 261731)
--- trunk/JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js (rev 0)
+++ trunk/JSTests/microbenchmarks/get-and-put-by-val-double-index-dont-fall-off-a-cliff.js 2020-05-15 06:55:08 UTC (rev 261731)
@@ -0,0 +1,47 @@
+const numPixels = 24000000;
+let source = new Uint8Array(numPixels);
+let target = new Uint8Array(numPixels);
+
+for (let i = 0; i < source.length; ++i) {
+ if (Math.random() > 0.35)
+ source[i] = 0;
+ else
+ source[i] = 1;
+}
+
+const area = {
+ x: asDoubleNumber(1.0),
+ y: asDoubleNumber(1.0),
+ x2: asDoubleNumber(1001.0),
+ y2: asDoubleNumber(1001.0),
+ width: asDoubleNumber(1000.0),
+ height: asDoubleNumber(1000.0),
+ segmentationWidth: asDoubleNumber(6000.0),
+ edgesCacheWidth: asDoubleNumber(6000.0),
+};
+
+function test(source, target, area) {
+ // fast implementation that can't handle edges of segmentation area
+ const segmentationWidth = area.segmentationWidth;
+ const edgesCacheWidth = area.edgesCacheWidth;
+ const {x2, y2} = area;
+
+ for (let y = area.y; y < y2; ++y) {
+ for (let x = area.x; x < x2; ++x) {
+ const sourceIndex = (y * segmentationWidth) + x;
+ const value = source[sourceIndex];
+
+ if (value !== source[sourceIndex - 1] ||
+ value !== source[sourceIndex + 1] ||
+ value !== source[sourceIndex - segmentationWidth] ||
+ value !== source[sourceIndex + segmentationWidth])
+ {
+ const targetIndex = (y * edgesCacheWidth) + x;
+ target[targetIndex] = 1;
+ }
+ }
+ }
+}
+noInline(test);
+
+test(source, target, area);
Modified: trunk/Source/_javascript_Core/ChangeLog (261730 => 261731)
--- trunk/Source/_javascript_Core/ChangeLog 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-05-15 06:55:08 UTC (rev 261731)
@@ -1,3 +1,35 @@
+2020-05-14 Saam Barati <[email protected]>
+
+ GetByVal and PutByVal runtime operations shouldn't fall off a performance cliff when the property is an integer boxed as a double
+ https://bugs.webkit.org/show_bug.cgi?id=211935
+
+ Reviewed by Yusuke Suzuki and Mark Lam.
+
+ There were parts in the runtime for get_by_val that weren't properly handling
+ ints boxed as doubles along the fast path. This could lead to terrible
+ performance as we could go from double -> string -> int while converting the
+ subscript into a property to access.
+
+ This patch fixes that, and removes the duplicate code we had throughout the
+ codebase that does this conversion. I'm adding a new functions tryGetAsUint32Index
+ and tryGetAsInt32 which will handle the double to int conversion.
+
+ This is a 10x speedup on the microbenchmark get-and-put-by-val-double-index-dont-fall-off-a-cliff.js
+
+ * dfg/DFGOperations.cpp:
+ (JSC::DFG::putByValInternal):
+ * jit/JITOperations.cpp:
+ (JSC::getByVal):
+ * jsc.cpp:
+ (functionAsDoubleNumber):
+ * llint/LLIntSlowPaths.cpp:
+ (JSC::LLInt::getByVal):
+ (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+ * runtime/JSCJSValue.h:
+ * runtime/JSCJSValueInlines.h:
+ (JSC::JSValue::tryGetAsUint32Index):
+ (JSC::JSValue::tryGetAsInt32):
+
2020-05-14 Devin Rousso <[email protected]>
[ESNext] enable logical assignment operators by default
Modified: trunk/Source/_javascript_Core/dfg/DFGOperations.cpp (261730 => 261731)
--- trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/dfg/DFGOperations.cpp 2020-05-15 06:55:08 UTC (rev 261731)
@@ -129,24 +129,12 @@
JSValue property = JSValue::decode(encodedProperty);
JSValue value = JSValue::decode(encodedValue);
- if (LIKELY(property.isUInt32())) {
- // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
- ASSERT(isIndex(property.asUInt32()));
+ if (Optional<uint32_t> index = property.tryGetAsUint32Index()) {
scope.release();
- putByVal<strict, direct>(globalObject, vm, baseValue, property.asUInt32(), value);
+ putByVal<strict, direct>(globalObject, vm, baseValue, *index, value);
return;
}
- if (property.isDouble()) {
- double propertyAsDouble = property.asDouble();
- uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
- if (propertyAsDouble == propertyAsUInt32 && isIndex(propertyAsUInt32)) {
- scope.release();
- putByVal<strict, direct>(globalObject, vm, baseValue, propertyAsUInt32, value);
- return;
- }
- }
-
// Don't put to an object if toString throws an exception.
auto propertyName = property.toPropertyKey(globalObject);
RETURN_IF_EXCEPTION(scope, void());
@@ -668,16 +656,10 @@
JSValue property = JSValue::decode(encodedProperty);
- if (property.isUInt32())
- RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, property.asUInt32()));
+ if (Optional<uint32_t> index = property.tryGetAsUint32Index())
+ RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, *index));
- if (property.isDouble()) {
- double propertyAsDouble = property.asDouble();
- uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
- if (propertyAsUInt32 == propertyAsDouble)
- RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, propertyAsUInt32));
-
- } else if (property.isString()) {
+ if (property.isString()) {
Structure& structure = *base->structure(vm);
if (JSCell::canUseFastGetOwnProperty(structure)) {
RefPtr<AtomStringImpl> existingAtomString = asString(property)->toExistingAtomString(globalObject);
@@ -1571,8 +1553,8 @@
}
PropertySlot slot(thisVal, PropertySlot::PropertySlot::InternalMethodType::Get);
- if (subscript.isUInt32()) {
- uint32_t i = subscript.asUInt32();
+ if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+ uint32_t i = *index;
if (isJSString(baseValue) && asString(baseValue)->canGetIndex(i))
return JSValue::encode(asString(baseValue)->getIndex(globalObject, i));
Modified: trunk/Source/_javascript_Core/jit/JITOperations.cpp (261730 => 261731)
--- trunk/Source/_javascript_Core/jit/JITOperations.cpp 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/jit/JITOperations.cpp 2020-05-15 06:55:08 UTC (rev 261731)
@@ -715,9 +715,9 @@
{
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
- if (LIKELY(subscript.isUInt32())) {
+ if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
byValInfo->tookSlowPath = true;
- uint32_t i = subscript.asUInt32();
+ uint32_t i = *index;
if (baseValue.isObject()) {
JSObject* object = asObject(baseValue);
if (object->canSetIndexQuickly(i, value)) {
@@ -734,7 +734,8 @@
scope.release();
baseValue.putByIndex(globalObject, i, value, ecmaMode.isStrict());
return;
- } else if (subscript.isInt32()) {
+ }
+ if (subscript.isInt32()) {
byValInfo->tookSlowPath = true;
if (baseValue.isObject())
byValInfo->arrayProfile->setOutOfBounds();
@@ -757,11 +758,9 @@
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
- if (LIKELY(subscript.isUInt32())) {
- // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
+ if (Optional<uint32_t> maybeIndex = subscript.tryGetAsUint32Index()) {
byValInfo->tookSlowPath = true;
- uint32_t index = subscript.asUInt32();
- ASSERT(isIndex(index));
+ uint32_t index = *maybeIndex;
switch (baseObject->indexingType()) {
case ALL_INT32_INDEXING_TYPES:
@@ -781,17 +780,6 @@
return;
}
- if (subscript.isDouble()) {
- double subscriptAsDouble = subscript.asDouble();
- uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
- if (subscriptAsDouble == subscriptAsUInt32 && isIndex(subscriptAsUInt32)) {
- byValInfo->tookSlowPath = true;
- scope.release();
- baseObject->putDirectIndex(globalObject, subscriptAsUInt32, value, 0, ecmaMode.isStrict() ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
- return;
- }
- }
-
// Don't put to an object if toString threw an exception.
auto property = subscript.toPropertyKey(globalObject);
RETURN_IF_EXCEPTION(scope, void());
@@ -1970,8 +1958,8 @@
}
}
- if (subscript.isInt32()) {
- int32_t i = subscript.asInt32();
+ if (Optional<int32_t> index = subscript.tryGetAsInt32()) {
+ int32_t i = *index;
if (isJSString(baseValue)) {
if (i >= 0 && asString(baseValue)->canGetIndex(i))
RELEASE_AND_RETURN(scope, asString(baseValue)->getIndex(globalObject, i));
@@ -2083,15 +2071,10 @@
if (LIKELY(baseValue.isCell())) {
JSCell* base = baseValue.asCell();
- if (property.isUInt32())
- RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, property.asUInt32()));
+ if (Optional<uint32_t> index = property.tryGetAsUint32Index())
+ RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, *index));
- if (property.isDouble()) {
- double propertyAsDouble = property.asDouble();
- uint32_t propertyAsUInt32 = static_cast<uint32_t>(propertyAsDouble);
- if (propertyAsUInt32 == propertyAsDouble && isIndex(propertyAsUInt32))
- RELEASE_AND_RETURN(scope, getByValWithIndex(globalObject, base, propertyAsUInt32));
- } else if (property.isString()) {
+ if (property.isString()) {
Structure& structure = *base->structure(vm);
if (JSCell::canUseFastGetOwnProperty(structure)) {
RefPtr<AtomStringImpl> existingAtomString = asString(property)->toExistingAtomString(globalObject);
Modified: trunk/Source/_javascript_Core/jsc.cpp (261730 => 261731)
--- trunk/Source/_javascript_Core/jsc.cpp 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/jsc.cpp 2020-05-15 06:55:08 UTC (rev 261731)
@@ -373,6 +373,7 @@
static EncodedJSValue JSC_HOST_CALL functionTotalCompileTime(JSGlobalObject*, CallFrame*);
static EncodedJSValue JSC_HOST_CALL functionSetUnhandledRejectionCallback(JSGlobalObject*, CallFrame*);
+static EncodedJSValue JSC_HOST_CALL functionAsDoubleNumber(JSGlobalObject*, CallFrame*);
struct Script {
enum class StrictMode {
@@ -641,6 +642,8 @@
addFunction(vm, "totalCompileTime", functionTotalCompileTime, 0);
addFunction(vm, "setUnhandledRejectionCallback", functionSetUnhandledRejectionCallback, 1);
+
+ addFunction(vm, "asDoubleNumber", functionAsDoubleNumber, 1);
}
void addFunction(VM& vm, JSObject* object, const char* name, NativeFunction function, unsigned arguments)
@@ -2492,6 +2495,15 @@
return JSValue::encode(jsUndefined());
}
+EncodedJSValue JSC_HOST_CALL functionAsDoubleNumber(JSGlobalObject* globalObject, CallFrame* callFrame)
+{
+ VM& vm = globalObject->vm();
+ auto scope = DECLARE_THROW_SCOPE(vm);
+ double num = callFrame->argument(0).toNumber(globalObject);
+ RETURN_IF_EXCEPTION(scope, encodedJSValue());
+ return JSValue::encode(jsDoubleNumber(num));
+}
+
// Use SEH for Release builds only to get rid of the crash report dialog
// (luckily the same tests fail in Release and Debug builds so far). Need to
// be in a separate main function because the jscmain function requires object
Modified: trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp (261730 => 261731)
--- trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/llint/LLIntSlowPaths.cpp 2020-05-15 06:55:08 UTC (rev 261731)
@@ -1012,8 +1012,8 @@
}
}
- if (subscript.isUInt32()) {
- uint32_t i = subscript.asUInt32();
+ if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+ uint32_t i = *index;
auto& metadata = bytecode.metadata(codeBlock);
ArrayProfile* arrayProfile = &metadata.m_arrayProfile;
@@ -1091,8 +1091,8 @@
JSValue value = getOperand(callFrame, bytecode.m_value);
bool isStrictMode = bytecode.m_ecmaMode.isStrict();
- if (LIKELY(subscript.isUInt32())) {
- uint32_t i = subscript.asUInt32();
+ if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+ uint32_t i = *index;
if (baseValue.isObject()) {
JSObject* object = asObject(baseValue);
if (object->canSetIndexQuickly(i, value))
@@ -1123,22 +1123,11 @@
RELEASE_ASSERT(baseValue.isObject());
JSObject* baseObject = asObject(baseValue);
bool isStrictMode = bytecode.m_ecmaMode.isStrict();
- if (LIKELY(subscript.isUInt32())) {
- // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices.
- ASSERT(isIndex(subscript.asUInt32()));
- baseObject->putDirectIndex(globalObject, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
+ if (Optional<uint32_t> index = subscript.tryGetAsUint32Index()) {
+ baseObject->putDirectIndex(globalObject, *index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
LLINT_END();
}
- if (subscript.isDouble()) {
- double subscriptAsDouble = subscript.asDouble();
- uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble);
- if (subscriptAsDouble == subscriptAsUInt32 && isIndex(subscriptAsUInt32)) {
- baseObject->putDirectIndex(globalObject, subscriptAsUInt32, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow);
- LLINT_END();
- }
- }
-
// Don't put to an object if toString threw an exception.
auto property = subscript.toPropertyKey(globalObject);
if (UNLIKELY(throwScope.exception()))
Modified: trunk/Source/_javascript_Core/runtime/JSCJSValue.h (261730 => 261731)
--- trunk/Source/_javascript_Core/runtime/JSCJSValue.h 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/runtime/JSCJSValue.h 2020-05-15 06:55:08 UTC (rev 261731)
@@ -216,6 +216,8 @@
int32_t asInt32() const;
uint32_t asUInt32() const;
+ Optional<uint32_t> tryGetAsUint32Index();
+ Optional<int32_t> tryGetAsInt32();
int64_t asAnyInt() const;
uint32_t asUInt32AsAnyInt() const;
int32_t asInt32AsAnyInt() const;
Modified: trunk/Source/_javascript_Core/runtime/JSCJSValueInlines.h (261730 => 261731)
--- trunk/Source/_javascript_Core/runtime/JSCJSValueInlines.h 2020-05-15 03:47:34 UTC (rev 261730)
+++ trunk/Source/_javascript_Core/runtime/JSCJSValueInlines.h 2020-05-15 06:55:08 UTC (rev 261731)
@@ -96,6 +96,34 @@
return isInt32() ? asInt32() : asDouble();
}
+inline Optional<uint32_t> JSValue::tryGetAsUint32Index()
+{
+ if (isUInt32()) {
+ ASSERT(isIndex(asUInt32()));
+ return asUInt32();
+ }
+ if (isNumber()) {
+ double number = asNumber();
+ uint32_t asUint = static_cast<uint32_t>(number);
+ if (static_cast<double>(asUint) == number && isIndex(asUint))
+ return asUint;
+ }
+ return WTF::nullopt;
+}
+
+inline Optional<int32_t> JSValue::tryGetAsInt32()
+{
+ if (isInt32())
+ return asInt32();
+ if (isNumber()) {
+ double number = asNumber();
+ int32_t asInt = static_cast<int32_t>(number);
+ if (static_cast<double>(asInt) == number)
+ return asInt;
+ }
+ return WTF::nullopt;
+}
+
inline JSValue jsNaN()
{
return JSValue(JSValue::EncodeAsDouble, PNaN);
