JSON.stringify should call replacer on deleted properties
authorshvaikalesh@gmail.com <shvaikalesh@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 7 Mar 2020 03:02:43 +0000 (03:02 +0000)
committershvaikalesh@gmail.com <shvaikalesh@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 7 Mar 2020 03:02:43 +0000 (03:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=208725

Reviewed by Ross Kirsling.

JSTests:

* microbenchmarks/json-stringify-many-objects-to-json.js: Added.
* microbenchmarks/json-stringify-many-objects.js: Added.
* test262/expectations.yaml: Mark 2 test cases as passing.

Source/JavaScriptCore:

This change removes extra `hasProperty` check from `appendNextProperty` as
it does not exist in the spec [1], aligning JSC with V8 and SpiderMonkey.

This patch also replaces 3 usages of `getPropertySlot` with semantically
equivalent (yet more concise) `get` and inlines `toJSONImpl` (this change
is performance-neutral).

[1]: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a)

* runtime/JSONObject.cpp:
(JSC::Stringifier::toJSON):
(JSC::Stringifier::Holder::appendNextProperty):
(JSC::Stringifier::toJSONImpl): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@258049 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/microbenchmarks/json-stringify-many-objects-to-json.js [new file with mode: 0644]
JSTests/microbenchmarks/json-stringify-many-objects.js [new file with mode: 0644]
JSTests/test262/expectations.yaml
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSONObject.cpp

index 4e3149d..6a84f7f 100644 (file)
@@ -1,3 +1,14 @@
+2020-03-06  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        JSON.stringify should call replacer on deleted properties
+        https://bugs.webkit.org/show_bug.cgi?id=208725
+
+        Reviewed by Ross Kirsling.
+
+        * microbenchmarks/json-stringify-many-objects-to-json.js: Added.
+        * microbenchmarks/json-stringify-many-objects.js: Added.
+        * test262/expectations.yaml: Mark 2 test cases as passing.
+
 2020-03-04  Mark Lam  <mark.lam@apple.com>
 
         Handle an out of memory error while constructing the BytecodeGenerator.
diff --git a/JSTests/microbenchmarks/json-stringify-many-objects-to-json.js b/JSTests/microbenchmarks/json-stringify-many-objects-to-json.js
new file mode 100644 (file)
index 0000000..9d53f2e
--- /dev/null
@@ -0,0 +1,6 @@
+const toJSON = () => '';
+const value = {};
+for (let i = 0; i < 100; ++i)
+    value[i] = {toJSON};
+for (let i = 0; i < 1e5 / 4; ++i)
+    JSON.stringify(value);
diff --git a/JSTests/microbenchmarks/json-stringify-many-objects.js b/JSTests/microbenchmarks/json-stringify-many-objects.js
new file mode 100644 (file)
index 0000000..5f083e1
--- /dev/null
@@ -0,0 +1,5 @@
+const value = {};
+for (let i = 0; i < 100; ++i)
+    value[i] = {};
+for (let i = 0; i < 1e5 / 4; ++i)
+    JSON.stringify(value);
index fadcad1..829fd6d 100644 (file)
@@ -1095,9 +1095,6 @@ test/built-ins/JSON/parse/reviver-array-non-configurable-prop-create.js:
 test/built-ins/JSON/parse/reviver-object-non-configurable-prop-create.js:
   default: 'Test262Error: Expected SameValue(«22», «2») to be true'
   strict mode: 'Test262Error: Expected SameValue(«22», «2») to be true'
-test/built-ins/JSON/stringify/replacer-function-object-deleted-property.js:
-  default: 'Test262Error: Expected SameValue(«{"a":1}», «{"a":1,"b":"<replaced>"}») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«{"a":1}», «{"a":1,"b":"<replaced>"}») to be true'
 test/built-ins/Map/proto-from-ctor-realm.js:
   default: 'Test262Error: Expected SameValue(«[object Map]», «[object Map]») to be true'
   strict mode: 'Test262Error: Expected SameValue(«[object Map]», «[object Map]») to be true'
index 9c639a7..117ead9 100644 (file)
@@ -1,3 +1,24 @@
+2020-03-06  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        JSON.stringify should call replacer on deleted properties
+        https://bugs.webkit.org/show_bug.cgi?id=208725
+
+        Reviewed by Ross Kirsling.
+
+        This change removes extra `hasProperty` check from `appendNextProperty` as
+        it does not exist in the spec [1], aligning JSC with V8 and SpiderMonkey.
+
+        This patch also replaces 3 usages of `getPropertySlot` with semantically
+        equivalent (yet more concise) `get` and inlines `toJSONImpl` (this change
+        is performance-neutral).
+
+        [1]: https://tc39.es/ecma262/#sec-serializejsonobject (steps 6, 8.a)
+
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::toJSON):
+        (JSC::Stringifier::Holder::appendNextProperty):
+        (JSC::Stringifier::toJSONImpl): Deleted.
+
 2020-03-06  Mark Lam  <mark.lam@apple.com>
 
         Fix some issues in the ARM64 moveConditionallyAfterFloatingPointCompare() and moveDoubleConditionallyAfterFloatingPointCompare().
index 99b3d08..c05cb73 100644 (file)
@@ -113,7 +113,6 @@ private:
     friend class Holder;
 
     JSValue toJSON(JSValue, const PropertyNameForFunctionCall&);
-    JSValue toJSONImpl(VM&, JSValue, JSValue toJSONFunction, const PropertyNameForFunctionCall&);
 
     enum StringifyResult { StringifyFailed, StringifySucceeded, StringifyFailedDueToUndefinedOrSymbolValue };
     StringifyResult appendStringifiedValue(StringBuilder&, JSValue, const Holder&, const PropertyNameForFunctionCall&);
@@ -302,19 +301,9 @@ ALWAYS_INLINE JSValue Stringifier::toJSON(JSValue baseValue, const PropertyNameF
     auto scope = DECLARE_THROW_SCOPE(vm);
     scope.assertNoException();
 
-    PropertySlot slot(baseValue, PropertySlot::InternalMethodType::Get);
-    bool hasProperty = baseValue.getPropertySlot(m_globalObject, vm.propertyNames->toJSON, slot);
-    EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
-    if (!hasProperty)
-        return baseValue;
-
-    JSValue toJSONFunction = slot.getValue(m_globalObject, vm.propertyNames->toJSON);
+    JSValue toJSONFunction = baseValue.get(m_globalObject, vm.propertyNames->toJSON);
     RETURN_IF_EXCEPTION(scope, { });
-    RELEASE_AND_RETURN(scope, toJSONImpl(vm, baseValue, toJSONFunction, propertyName));
-}
 
-JSValue Stringifier::toJSONImpl(VM& vm, JSValue baseValue, JSValue toJSONFunction, const PropertyNameForFunctionCall& propertyName)
-{
     CallType callType;
     CallData callData;
     if (!toJSONFunction.isCallable(vm, callType, callData))
@@ -528,13 +517,7 @@ bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBui
         if (m_isJSArray && m_object->canGetIndexQuickly(index))
             value = m_object->getIndexQuickly(index);
         else {
-            PropertySlot slot(m_object, PropertySlot::InternalMethodType::Get);
-            bool hasProperty = m_object->getPropertySlot(globalObject, index, slot);
-            EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
-            if (hasProperty)
-                value = slot.getValue(globalObject, index);
-            else
-                value = jsUndefined();
+            value = m_object->get(globalObject, index);
             RETURN_IF_EXCEPTION(scope, false);
         }
 
@@ -548,13 +531,8 @@ bool Stringifier::Holder::appendNextProperty(Stringifier& stringifier, StringBui
         ASSERT(stringifyResult != StringifyFailedDueToUndefinedOrSymbolValue);
     } else {
         // Get the value.
-        PropertySlot slot(m_object, PropertySlot::InternalMethodType::Get);
         Identifier& propertyName = m_propertyNames->propertyNameVector()[index];
-        bool hasProperty = m_object->getPropertySlot(globalObject, propertyName, slot);
-        EXCEPTION_ASSERT(!scope.exception() || !hasProperty);
-        if (!hasProperty)
-            return true;
-        JSValue value = slot.getValue(globalObject, propertyName);
+        JSValue value = m_object->get(globalObject, propertyName);
         RETURN_IF_EXCEPTION(scope, false);
 
         rollBackPoint = builder.length();