- Revision
- 271305
- Author
- [email protected]
- Date
- 2021-01-08 10:52:28 -0800 (Fri, 08 Jan 2021)
Log Message
for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
https://bugs.webkit.org/show_bug.cgi?id=189034
Reviewed by Yusuke Suzuki.
JSTests:
* microbenchmarks/for-in-proxy.js: Added.
* stress/for-in-redefine-enumerable.js:
* stress/proxy-for-in.js: Added.
Source/_javascript_Core:
Although the spec [1] doesn't normatively require calling [[GetOwnProperty]]
only once per property, this is what V8 and SpiderMonkey do.
Since [[Enumerable]] property attribute is checked by has_enumerable_property
bytecode op, this patch avoids another observable [[GetOwnProperty]] call
by using DontEnumPropertiesMode::Include exclusively for Proxy objects.
A side effect of this change: if a property becomes [[Enumerable]] after
[[OwnPropertyKeys]] trap was called, it will be enumerated, which matches
the spec [2] and developer expectations.
This patch advances provided microbenchmark by 100%.
[1]: https://tc39.es/ecma262/#sec-enumerate-object-properties
[2]: https://tc39.es/ecma262/#sec-%foriniteratorprototype%.next (step 7.b.iii)
* runtime/JSPropertyNameEnumerator.cpp:
(JSC::getEnumerablePropertyNames):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (271304 => 271305)
--- trunk/JSTests/ChangeLog 2021-01-08 18:46:37 UTC (rev 271304)
+++ trunk/JSTests/ChangeLog 2021-01-08 18:52:28 UTC (rev 271305)
@@ -1,3 +1,14 @@
+2021-01-08 Alexey Shvayka <[email protected]>
+
+ for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
+ https://bugs.webkit.org/show_bug.cgi?id=189034
+
+ Reviewed by Yusuke Suzuki.
+
+ * microbenchmarks/for-in-proxy.js: Added.
+ * stress/for-in-redefine-enumerable.js:
+ * stress/proxy-for-in.js: Added.
+
2021-01-08 Dmitry Bezhetskov <[email protected]>
[WASM-References] Add optional default value parameter for Table.constructor, Table.grow and Table.set
Added: trunk/JSTests/microbenchmarks/for-in-proxy.js (0 => 271305)
--- trunk/JSTests/microbenchmarks/for-in-proxy.js (rev 0)
+++ trunk/JSTests/microbenchmarks/for-in-proxy.js 2021-01-08 18:52:28 UTC (rev 271305)
@@ -0,0 +1,18 @@
+(function() {
+ var target = {};
+ for (var i = 0; i < 200; i++)
+ target["k" + i] = i;
+
+ var proxy = new Proxy(target, {});
+ var x = 0;
+
+ for (var j = 0; j < 1200; j++) {
+ for (var k in proxy) {
+ if (k === "k100")
+ x++;
+ }
+ }
+
+ if (x !== 1200)
+ throw new Error("Bad assertion!");
+})();
Modified: trunk/JSTests/stress/for-in-redefine-enumerable.js (271304 => 271305)
--- trunk/JSTests/stress/for-in-redefine-enumerable.js 2021-01-08 18:46:37 UTC (rev 271304)
+++ trunk/JSTests/stress/for-in-redefine-enumerable.js 2021-01-08 18:52:28 UTC (rev 271305)
@@ -73,7 +73,7 @@
}
shouldBe(target.a, 1);
shouldBe(target.b, 0);
- shouldBe(target.c, 0);
+ shouldBe(target.c, 1);
}
for (var i = 0; i < 1e5; ++i)
Added: trunk/JSTests/stress/proxy-for-in.js (0 => 271305)
--- trunk/JSTests/stress/proxy-for-in.js (rev 0)
+++ trunk/JSTests/stress/proxy-for-in.js 2021-01-08 18:52:28 UTC (rev 271305)
@@ -0,0 +1,120 @@
+// Author: Caitlin Potter <[email protected]>
+"use strict";
+
+function assert(b, m = "Bad assertion") {
+ if (!b)
+ throw new Error(m);
+}
+
+assert.eq = function eq(expected, actual, m = `Expected «${actual}» to be «${expected}».`) {
+ assert(expected === actual, m);
+}
+
+assert.arrEq = function arrEq(expected, actual, m = `Expected «${actual}» to be «${expected}».`) {
+ assert(Array.isArray(expected));
+ assert(Array.isArray(actual), `Expected «${actual}» to be an Array.`);
+ assert.eq(expected.length, actual.length, `Expected «${actual}» to have ${expected.length} items`);
+ for (let i = 0; i < expected.length; ++i) {
+ let e = expected[i], a = actual[i];
+ if (Array.isArray(e))
+ assert.arrEq(e, a);
+ else
+ assert.eq(expected[i], actual[i]);
+ }
+}
+
+// Enumerable, no ownKeys trap
+{
+ let log = [];
+ let p = new Proxy(Object.create(null, {
+ x: {
+ enumerable: true,
+ configurable: true,
+ value: 0
+ },
+ }), {
+ getOwnPropertyDescriptor(target, pname) {
+ log.push(`gopd ${pname}`);
+ return Reflect.getOwnPropertyDescriptor(target, pname);
+ }
+ });
+ for (let k in p) log.push(`enumerate ${k}`);
+
+ assert.arrEq([
+ "gopd x",
+ "enumerate x",
+ ], log);
+}
+
+// Non-enumerable, no ownKeys trap
+{
+ let log = [];
+ let p = new Proxy(Object.create(null, {
+ x: {
+ enumerable: false,
+ configurable: true,
+ value: 0
+ },
+ }), {
+ getOwnPropertyDescriptor(target, pname) {
+ log.push(`gopd ${pname}`);
+ return Reflect.getOwnPropertyDescriptor(target, pname);
+ }
+ });
+ for (let k in p) log.push(`enumerate ${k}`);
+
+ assert.arrEq([
+ "gopd x",
+ ], log);
+}
+
+// Enumerable, with ownKeys trap
+{
+ let log = [];
+ let p = new Proxy(Object.create(null, {
+ x: {
+ enumerable: true,
+ configurable: true,
+ value: 0
+ },
+ }), {
+ getOwnPropertyDescriptor(target, pname) {
+ log.push(`gopd ${pname}`);
+ return Reflect.getOwnPropertyDescriptor(target, pname);
+ },
+ ownKeys(target) {
+ return Reflect.ownKeys(target);
+ }
+ });
+ for (let k in p) log.push(`enumerate ${k}`);
+
+ assert.arrEq([
+ "gopd x",
+ "enumerate x",
+ ], log);
+}
+
+// Non-enumerable, with ownKeys trap
+{
+ let log = [];
+ let p = new Proxy(Object.create(null, {
+ x: {
+ enumerable: false,
+ configurable: true,
+ value: 0
+ },
+ }), {
+ getOwnPropertyDescriptor(target, pname) {
+ log.push(`gopd ${pname}`);
+ return Reflect.getOwnPropertyDescriptor(target, pname);
+ },
+ ownKeys(target) {
+ return Reflect.ownKeys(target);
+ }
+ });
+ for (let k in p) log.push(`enumerate ${k}`);
+
+ assert.arrEq([
+ "gopd x",
+ ], log);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (271304 => 271305)
--- trunk/Source/_javascript_Core/ChangeLog 2021-01-08 18:46:37 UTC (rev 271304)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-01-08 18:52:28 UTC (rev 271305)
@@ -1,3 +1,29 @@
+2021-01-08 Alexey Shvayka <[email protected]>
+
+ for/in over a Proxy should not call [[GetOwnProperty]] trap twice per property
+ https://bugs.webkit.org/show_bug.cgi?id=189034
+
+ Reviewed by Yusuke Suzuki.
+
+ Although the spec [1] doesn't normatively require calling [[GetOwnProperty]]
+ only once per property, this is what V8 and SpiderMonkey do.
+
+ Since [[Enumerable]] property attribute is checked by has_enumerable_property
+ bytecode op, this patch avoids another observable [[GetOwnProperty]] call
+ by using DontEnumPropertiesMode::Include exclusively for Proxy objects.
+
+ A side effect of this change: if a property becomes [[Enumerable]] after
+ [[OwnPropertyKeys]] trap was called, it will be enumerated, which matches
+ the spec [2] and developer expectations.
+
+ This patch advances provided microbenchmark by 100%.
+
+ [1]: https://tc39.es/ecma262/#sec-enumerate-object-properties
+ [2]: https://tc39.es/ecma262/#sec-%foriniteratorprototype%.next (step 7.b.iii)
+
+ * runtime/JSPropertyNameEnumerator.cpp:
+ (JSC::getEnumerablePropertyNames):
+
2021-01-08 Yusuke Suzuki <[email protected]>
Unreviewed, add missing scope.release() in JSModuleNamespaceObject
Modified: trunk/Source/_javascript_Core/runtime/JSPropertyNameEnumerator.cpp (271304 => 271305)
--- trunk/Source/_javascript_Core/runtime/JSPropertyNameEnumerator.cpp 2021-01-08 18:46:37 UTC (rev 271304)
+++ trunk/Source/_javascript_Core/runtime/JSPropertyNameEnumerator.cpp 2021-01-08 18:52:28 UTC (rev 271305)
@@ -94,6 +94,16 @@
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
+ auto getOwnPropertyNames = [&](JSObject* object) {
+ auto mode = DontEnumPropertiesMode::Exclude;
+ if (object->type() == ProxyObjectType) {
+ // This ensures Proxy's [[GetOwnProperty]] trap is invoked only once per property, by OpHasEnumerableProperty.
+ // Although doing this for all objects is spec-conformant, collecting DontEnum properties isn't free.
+ mode = DontEnumPropertiesMode::Include;
+ }
+ object->methodTable(vm)->getOwnPropertyNames(object, globalObject, propertyNames, mode);
+ };
+
Structure* structure = base->structure(vm);
if (structure->canAccessPropertiesQuicklyForEnumeration() && indexedLength == base->getArrayLength()) {
// Inlined JSObject::getOwnNonIndexPropertyNames()
@@ -109,7 +119,7 @@
if (!nonStructurePropertyCount)
structurePropertyCount = propertyNames.size();
} else {
- base->methodTable(vm)->getOwnPropertyNames(base, globalObject, propertyNames, DontEnumPropertiesMode::Exclude);
+ getOwnPropertyNames(base);
RETURN_IF_EXCEPTION(scope, void());
// |propertyNames| contains all indexed properties, so disable enumeration based on getEnumerableLength().
indexedLength = 0;
@@ -130,7 +140,7 @@
}
object = asObject(prototype);
- object->methodTable(vm)->getOwnPropertyNames(object, globalObject, propertyNames, DontEnumPropertiesMode::Exclude);
+ getOwnPropertyNames(object);
RETURN_IF_EXCEPTION(scope, void());
}
}
