Skip to content

Commit 8aac9ec

Browse files
shvaikaleshshvaikalesh
committed
[JSC] Simplify get*PropertyNames() methods and EnumerationMode
https://bugs.webkit.org/show_bug.cgi?id=212954 Reviewed by Yusuke Suzuki. JSTests: * ChakraCore.yaml: * ChakraCore/test/Basics/enum.baseline-jsc: Removed. * microbenchmarks/for-in-on-object-with-lazily-materialized-properties.js: Removed because ErrorInstance no longer materializes properties during for/in enumeration. * microbenchmarks/object-keys-cloned-arguments.js: Added. * microbenchmarks/object-keys-error-object.js: Added. * stress/arguments-properties-order.js: Added. * stress/for-in-tests.js: * stress/for-in-typed-array.js: Source/JavaScriptCore: Before this change, [[OwnPropertyKeys]] overrides were sometimes implemented inconsistently, via different get*PropertyNames() methods that duplicated logic (e.g. ErrorInstance, RegExpObject, and StringObject). This patch: 1. Introduces a clear convention to implement [[OwnPropertyKeys]] overrides: if it's defined by the spec, getOwnPropertyNames() method is used; otherwise, non-materialized properties are enumerated / reified in getOwnSpecialPropertyNames(). While no class should define both methods, we don't assert this to support inheritance. Removes getOwnNonIndexPropertyNames() from the method table and converts it to instance method; its overrides were renamed to getOwnSpecialPropertyNames() and exempted from calling the no-op base method. This approach was chosen, instead of getOwnNonIndexPropertyNames() override, because for/in enumeration must be sure there are no enumerable properties between getEnumerableLength() and the first structure property. Also, removes getStructurePropertyNames() from the method table as it's unreasonable to override it. 2. Extracts JSObject::getOwnIndexPropertyNames() instance method to enforce correct enumeration order in getOwnPropertyNames() overrides: special indices => butterfly storage => special properties => non-reified static => structure properties. Loose mode `arguments` were fixed to enumerate indices from butterfly storage before special properties [1], aligning JSC with V8 and SpiderMonkey. 3. Reworks for/in enumeration so the special properties always come before structure ones, aligning enumeration order of String objects [2] and typed arrays [3] that have expando properties with the spec, V8, and SpiderMonkey. Removes getPropertyNames() and getGenericPropertyNames() from the method table, along with their overrides, because ES7 disabled customization of for/in enumeration [4]. Instead, JSObject::getPropertyNames() instance method and getEnumerablePropertyNames() are introduced, featuring a loop instead of recursion. Also, this enabled dropping hard-to-follow JSObjectPropertiesMode bit and simplifying EnumerationMode to an enum. for/in and Object.keys microbenchmarks are neutral. This change does not affect JSPropertyNameEnumerator caching, nor fast paths of its bytecodes. [1]: https://tc39.es/ecma262/#sec-createmappedargumentsobject (steps 15-16 and 20-21) [2]: https://tc39.es/ecma262/#sec-string-exotic-objects-ownpropertykeys [3]: https://tc39.es/ecma262/#sec-integer-indexed-exotic-objects-ownpropertykeys [4]: tc39/ecma262#367 * API/JSAPIValueWrapper.h: Remove OverridesAnyFormOfGetPropertyNames structure flag as it should never be queried from JSCell instances. * API/JSCallbackObject.h: * API/JSCallbackObjectFunctions.h: (JSC::JSCallbackObject<Parent>::getOwnSpecialPropertyNames): (JSC::JSCallbackObject<Parent>::getOwnNonIndexPropertyNames): Deleted. * API/JSObjectRef.cpp: (JSObjectCopyPropertyNames): * bindings/ScriptValue.cpp: (Inspector::jsToInspectorValue): * bytecode/ObjectAllocationProfileInlines.h: (JSC::ObjectAllocationProfileBase<Derived>::possibleDefaultPropertyCount): Use DontEnumPropertyMode::Include as the intent is to count all properties, even private symbols. EnumerationMode() defaults did exclude non-enumerable properties. * debugger/DebuggerScope.cpp: (JSC::DebuggerScope::getOwnPropertyNames): * debugger/DebuggerScope.h: * runtime/ClassInfo.h: * runtime/ClonedArguments.cpp: (JSC::ClonedArguments::getOwnSpecialPropertyNames): Don't materialize DontEnum properties unless it's DontEnumPropertiesMode::Include, advancing provided microbenchmark by ~23%. (JSC::ClonedArguments::getOwnPropertyNames): Deleted. * runtime/ClonedArguments.h: * runtime/EnumerationMode.h: Explicitly specify enum type to reduce its size. (JSC::EnumerationMode::EnumerationMode): Deleted. (JSC::EnumerationMode::includeDontEnumProperties): Deleted. (JSC::EnumerationMode::includeJSObjectProperties): Deleted. * runtime/ErrorInstance.cpp: (JSC::ErrorInstance::getOwnSpecialPropertyNames): Don't materialize DontEnum properties unless it's DontEnumPropertiesMode::Include, advancing provided microbenchmark by a factor of 5. (JSC::ErrorInstance::getOwnNonIndexPropertyNames): Deleted. (JSC::ErrorInstance::getStructurePropertyNames): Deleted. * runtime/ErrorInstance.h: * runtime/GenericArguments.h: * runtime/GenericArgumentsInlines.h: (JSC::GenericArguments<Type>::getOwnPropertyNames): * runtime/JSArray.cpp: (JSC::JSArray::getOwnSpecialPropertyNames): (JSC::JSArray::getOwnNonIndexPropertyNames): Deleted. * runtime/JSArray.h: * runtime/JSCell.cpp: (JSC::JSCell::getOwnPropertyNames): (JSC::JSCell::getOwnSpecialPropertyNames): (JSC::JSCell::getOwnNonIndexPropertyNames): Deleted. (JSC::JSCell::getPropertyNames): Deleted. (JSC::JSCell::getStructurePropertyNames): Deleted. (JSC::JSCell::getGenericPropertyNames): Deleted. * runtime/JSCell.h: * runtime/JSFunction.cpp: (JSC::JSFunction::getOwnSpecialPropertyNames): (JSC::JSFunction::getOwnNonIndexPropertyNames): Deleted. * runtime/JSFunction.h: * runtime/JSGenericTypedArrayView.h: * runtime/JSGenericTypedArrayViewInlines.h: (JSC::JSGenericTypedArrayView<Adaptor>::getOwnPropertyNames): * runtime/JSGlobalObject.h: Remove OverridesAnyFormOfGetPropertyNames structure flag as it's inherited from JSSymbolTableObject, and JSGlobalObject itself doesn't override getOwn*PropertyNames(). * runtime/JSLexicalEnvironment.cpp: (JSC::JSLexicalEnvironment::getOwnSpecialPropertyNames): (JSC::JSLexicalEnvironment::getOwnNonIndexPropertyNames): Deleted. * runtime/JSLexicalEnvironment.h: * runtime/JSModuleEnvironment.cpp: (JSC::JSModuleEnvironment::getOwnSpecialPropertyNames): (JSC::JSModuleEnvironment::getOwnNonIndexPropertyNames): Deleted. * runtime/JSModuleEnvironment.h: * runtime/JSModuleNamespaceObject.cpp: (JSC::JSModuleNamespaceObject::getOwnPropertyNames): Call getOwnNonIndexPropertyNames() directly, guarded by includeSymbolProperties() check, since module namespace objects can't have string properties besides m_names. (See https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-defineownproperty-p-desc) * runtime/JSModuleNamespaceObject.h: * runtime/JSONObject.cpp: (JSC::Stringifier::Holder::appendNextProperty): (JSC::Walker::walk): * runtime/JSObject.cpp: (JSC::JSObject::getNonReifiedStaticPropertyNames): (JSC::JSObject::getPropertyNames): (JSC::JSObject::getOwnPropertyNames): (JSC::JSObject::getOwnSpecialPropertyNames): (JSC::JSObject::getOwnIndexedPropertyNames): (JSC::JSObject::getOwnNonIndexPropertyNames): (JSC::getClassPropertyNames): Deleted. (JSC::JSObject::getStructurePropertyNames): Deleted. (JSC::JSObject::getGenericPropertyNames): Deleted. * runtime/JSObject.h: (JSC::JSObject::getOwnSpecialPropertyNames): * runtime/JSPropertyNameEnumerator.cpp: (JSC::getEnumerablePropertyNames): * runtime/JSPropertyNameEnumerator.h: (JSC::propertyNameEnumerator): * runtime/JSProxy.cpp: (JSC::JSProxy::getOwnPropertyNames): (JSC::JSProxy::getPropertyNames): Deleted. (JSC::JSProxy::getStructurePropertyNames): Deleted. (JSC::JSProxy::getGenericPropertyNames): Deleted. * runtime/JSProxy.h: * runtime/JSSymbolTableObject.cpp: (JSC::JSSymbolTableObject::getOwnSpecialPropertyNames): (JSC::JSSymbolTableObject::getOwnNonIndexPropertyNames): Deleted. * runtime/JSSymbolTableObject.h: * runtime/JSTypeInfo.h: (JSC::TypeInfo::overridesGetOwnPropertyNames const): (JSC::TypeInfo::overridesGetOwnSpecialPropertyNames const): (JSC::TypeInfo::overridesAnyFormOfGetOwnPropertyNames const): (JSC::TypeInfo::overridesGetPropertyNames const): Deleted. (JSC::TypeInfo::overridesAnyFormOfGetPropertyNames const): Deleted. * runtime/ObjectConstructor.cpp: (JSC::objectConstructorGetOwnPropertyDescriptors): (JSC::JSC_DEFINE_HOST_FUNCTION): (JSC::defineProperties): (JSC::setIntegrityLevel): (JSC::testIntegrityLevel): (JSC::ownPropertyKeys): * runtime/ProxyObject.cpp: (JSC::ProxyObject::performGetOwnPropertyNames): (JSC::ProxyObject::getOwnPropertyNames): (JSC::ProxyObject::getPropertyNames): Deleted. (JSC::ProxyObject::getOwnNonIndexPropertyNames): Deleted. (JSC::ProxyObject::getStructurePropertyNames): Deleted. (JSC::ProxyObject::getGenericPropertyNames): Deleted. * runtime/ProxyObject.h: Remove IsQuickPropertyAccessAllowedForEnumeration flag from ProxyObject's structure since canAccessPropertiesQuicklyForEnumeration() now checks for method overrides. * runtime/RegExpObject.cpp: (JSC::RegExpObject::getOwnSpecialPropertyNames): (JSC::RegExpObject::getOwnNonIndexPropertyNames): Deleted. (JSC::RegExpObject::getPropertyNames): Deleted. (JSC::RegExpObject::getGenericPropertyNames): Deleted. * runtime/RegExpObject.h: * runtime/StringObject.cpp: (JSC::StringObject::getOwnPropertyNames): (JSC::StringObject::getOwnNonIndexPropertyNames): Deleted. * runtime/StringObject.h: * runtime/Structure.cpp: (JSC::Structure::validateFlags): Strengthen overridesGetOwn*PropertyNames and overridesGetPrototype asserts into equivalence tests. (JSC::Structure::getPropertyNamesFromStructure): (JSC::Structure::canAccessPropertiesQuicklyForEnumeration const): * runtime/Structure.h: * runtime/StructureInlines.h: (JSC::Structure::canCacheOwnPropertyNames const): * tools/JSDollarVM.cpp: Remove OverridesAnyFormOfGetPropertyNames structure flag as it's inherited from JSArray, and RuntimeArray itself doesn't override getOwn*PropertyNames(). Source/WebCore: Adjust for changes in JSC's MethodTable, TypeInfo, and EnumerationMode. No new tests, no behavior change. * animation/KeyframeEffect.cpp: (WebCore::processKeyframeLikeObject): * bindings/js/JSDOMConvertRecord.h: * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::getOwnPropertyNames): * bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::getOwnPropertyNames): * bindings/js/JSRemoteDOMWindowCustom.cpp: (WebCore::JSRemoteDOMWindow::getOwnPropertyNames): * bindings/js/SerializedScriptValue.cpp: (WebCore::CloneSerializer::serialize): * bindings/scripts/CodeGeneratorJS.pm: (GenerateGetOwnPropertyNames): (GenerateHeader): * bindings/scripts/test/JS/*: Updated. * bridge/NP_jsobject.cpp: * bridge/runtime_array.cpp: (JSC::RuntimeArray::getOwnPropertyNames): * bridge/runtime_array.h: * bridge/runtime_object.cpp: (JSC::Bindings::RuntimeObject::getOwnPropertyNames): * bridge/runtime_object.h: Source/WebKit: Adjust for changes in JSC's MethodTable, TypeInfo, and EnumerationMode. No new tests, no behavior change. * WebProcess/Plugins/Netscape/JSNPObject.cpp: (WebKit::JSNPObject::getOwnPropertyNames): * WebProcess/Plugins/Netscape/JSNPObject.h: * WebProcess/Plugins/Netscape/NPJSObject.cpp: (WebKit::NPJSObject::enumerate): Source/WebKitLegacy/mac: * Plugins/Hosted/NetscapePluginInstanceProxy.mm: (WebKit::NetscapePluginInstanceProxy::enumerate): Canonical link: https://commits.webkit.org/232854@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271269 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent eba1d91 commit 8aac9ec

File tree

129 files changed

+749
-639
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

129 files changed

+749
-639
lines changed

‎JSTests/ChakraCore.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@
303303
- path: ChakraCore/test/Basics/scan.js
304304
cmd: runChakra :baseline, "NoException", "scan.baseline-jsc", []
305305
- path: ChakraCore/test/Basics/enum.js
306-
cmd: runChakra :baseline, "NoException", "enum.baseline-jsc", []
306+
cmd: runChakra :pass, "NoException", "", []
307307
- path: ChakraCore/test/Basics/with3.js
308308
cmd: runChakra :baseline, "NoException", "with3.baseline-jsc", []
309309
- path: ChakraCore/test/Basics/cross_site_accessor_main.js

‎JSTests/ChakraCore/test/Basics/enum.baseline-jsc‎

Lines changed: 0 additions & 117 deletions
This file was deleted.

‎JSTests/ChangeLog‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,21 @@
1+
2021-01-07 Alexey Shvayka <shvaikalesh@gmail.com>
2+
3+
[JSC] Simplify get*PropertyNames() methods and EnumerationMode
4+
https://bugs.webkit.org/show_bug.cgi?id=212954
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
* ChakraCore.yaml:
9+
* ChakraCore/test/Basics/enum.baseline-jsc: Removed.
10+
* microbenchmarks/for-in-on-object-with-lazily-materialized-properties.js:
11+
Removed because ErrorInstance no longer materializes properties during for/in enumeration.
12+
13+
* microbenchmarks/object-keys-cloned-arguments.js: Added.
14+
* microbenchmarks/object-keys-error-object.js: Added.
15+
* stress/arguments-properties-order.js: Added.
16+
* stress/for-in-tests.js:
17+
* stress/for-in-typed-array.js:
18+
119
2021-01-07 Yusuke Suzuki <ysuzuki@apple.com>
220

321
[JSC] New expression and value function call should reserve function register if arguments include assignments

‎JSTests/microbenchmarks/for-in-on-object-with-lazily-materialized-properties.js‎

Lines changed: 0 additions & 15 deletions
This file was deleted.

‎JSTests/microbenchmarks/object-keys-cloned-arguments.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
noInline(Object.keys);
2+
3+
function getArgs(a, b, c) {
4+
"use strict";
5+
return arguments;
6+
}
7+
8+
for (var i = 0; i < 2e5; ++i)
9+
Object.keys(getArgs(1, 2, 3));

‎JSTests/microbenchmarks/object-keys-error-object.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
noInline(Object.keys);
2+
3+
for (var i = 0; i < 2e5; ++i)
4+
Object.keys(new Error());

‎JSTests/stress/arguments-properties-order.js‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
function getMappedArguments(a, b) { return arguments; }
2+
function getUnmappedArguments(a, b) { "use strict"; return arguments; }
3+
4+
function shouldBeArray(actual, expected) {
5+
var isEqual =
6+
actual.length === expected.length &&
7+
actual.every((item, index) => item === expected[index]);
8+
if (!isEqual)
9+
throw new Error(`Expected [${actual.map(String)}] to equal [${expected.map(String)}]`);
10+
}
11+
12+
function forIn(object) {
13+
var keys = [];
14+
for (var key in object)
15+
keys.push(key);
16+
return keys;
17+
}
18+
19+
noInline(getMappedArguments);
20+
noInline(getUnmappedArguments);
21+
noInline(forIn);
22+
23+
(function() {
24+
for (var i = 0; i < 1e4; ++i) {
25+
var mappedArguments = getMappedArguments(0, 1, 2);
26+
shouldBeArray(forIn(mappedArguments), ["0", "1", "2"]);
27+
shouldBeArray(Object.keys(mappedArguments), ["0", "1", "2"]);
28+
shouldBeArray(Reflect.ownKeys(mappedArguments), ["0", "1", "2", "length", "callee", Symbol.iterator]);
29+
30+
var unmappedArguments = getUnmappedArguments(0);
31+
shouldBeArray(forIn(unmappedArguments), ["0"]);
32+
shouldBeArray(Object.keys(unmappedArguments), ["0"]);
33+
shouldBeArray(Reflect.ownKeys(unmappedArguments), ["0", "length", "callee", Symbol.iterator]);
34+
}
35+
})();
36+
37+
(function() {
38+
for (var i = 0; i < 1e4; ++i) {
39+
var mappedArguments = getMappedArguments(0, 1);
40+
mappedArguments[8] = 8;
41+
mappedArguments[2] = 2;
42+
shouldBeArray(forIn(mappedArguments), ["0", "1", "2", "8"]);
43+
shouldBeArray(Object.keys(mappedArguments), ["0", "1", "2", "8"]);
44+
shouldBeArray(Reflect.ownKeys(mappedArguments), ["0", "1", "2", "8", "length", "callee", Symbol.iterator]);
45+
46+
var unmappedArguments = getUnmappedArguments();
47+
unmappedArguments[12] = 12;
48+
unmappedArguments[3] = 3;
49+
shouldBeArray(forIn(unmappedArguments), ["3", "12"]);
50+
shouldBeArray(Object.keys(unmappedArguments), ["3", "12"]);
51+
shouldBeArray(Reflect.ownKeys(unmappedArguments), ["3", "12", "length", "callee", Symbol.iterator]);
52+
}
53+
})();
54+
55+
(function() {
56+
for (var i = 0; i < 1e4; ++i) {
57+
var mappedArguments = getMappedArguments(0);
58+
mappedArguments.foo = 1;
59+
mappedArguments.bar = 2;
60+
shouldBeArray(forIn(mappedArguments), ["0", "foo", "bar"]);
61+
shouldBeArray(Object.keys(mappedArguments), ["0", "foo", "bar"]);
62+
// FIXME: Symbol.iterator should come after "foo" and "bar"
63+
// shouldBeArray(Reflect.ownKeys(mappedArguments), ["0", "length", "callee", "foo", "bar", Symbol.iterator]);
64+
65+
var unmappedArguments = getUnmappedArguments(0, 1, 2);
66+
unmappedArguments.foo = 1;
67+
unmappedArguments.bar = 2;
68+
shouldBeArray(forIn(unmappedArguments), ["0", "1", "2", "foo", "bar"]);
69+
shouldBeArray(Object.keys(unmappedArguments), ["0", "1", "2", "foo", "bar"]);
70+
// FIXME: "callee" should come before "foo" and "bar"
71+
// shouldBeArray(Reflect.ownKeys(unmappedArguments), ["0", "1", "2", "length", "callee", "foo", "bar", Symbol.iterator]);
72+
}
73+
})();
74+
75+
// FIXME: Add more tests, covering:
76+
// * added symbol properties;
77+
// * added together index, non-index, and symbol properties;
78+
// * deleted, re-added, and redefined as DontEnum index properties, both within and beyond "length";
79+
// * deleted, re-added, and redefined as DontEnum "length", "callee", and Symbol.iterator properties.

‎JSTests/stress/for-in-tests.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,24 @@ function shouldThrowSyntaxError(script) {
8787
}
8888
foo(null);
8989
})();
90+
(function() {
91+
// Iterate over an object with non-reified static property names & structure property
92+
if (typeof WebAssembly === "undefined")
93+
return;
94+
95+
WebAssembly.foo = 1;
96+
97+
function forIn() {
98+
for (var key in WebAssembly) {}
99+
return key;
100+
}
101+
noInline(forIn);
102+
103+
for (var i = 0; i < 10000; ++i) {
104+
if (forIn() !== "foo")
105+
throw new Error("bad result");
106+
}
107+
})();
90108
(function() {
91109
var foo = function(a, b) {
92110
for (var p in b) {

‎JSTests/stress/for-in-typed-array.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,25 @@
1616
}
1717
foo(null);
1818
})();
19+
20+
(function() {
21+
function forIn() {
22+
var a = new Int32Array(4);
23+
a.foo = 1;
24+
a.bar = 2;
25+
for (var i = 0; i < a.length; ++i)
26+
a[i] = i;
27+
28+
var keys = [];
29+
for (var k in a)
30+
keys.push(k);
31+
return keys.join("|");
32+
}
33+
noInline(forIn);
34+
35+
for (var i = 0; i < 1e4; ++i) {
36+
var keys = forIn();
37+
if (keys !== "0|1|2|3|foo|bar")
38+
throw new Error(`Bad result: ${keys}`);
39+
}
40+
})();

‎Source/JavaScriptCore/API/JSAPIValueWrapper.h‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,7 @@ class JSAPIValueWrapper final : public JSCell {
3333
friend JSValue jsAPIValueWrapper(JSGlobalObject*, JSValue);
3434
public:
3535
using Base = JSCell;
36-
37-
// OverridesAnyFormOfGetPropertyNames (which used to be OverridesGetPropertyNames) was here
38-
// since ancient times back when we pessimistically choose to apply this flag. I think we
39-
// can remove it, but we should do more testing before we do so.
40-
// Ref: http://trac.webkit.org/changeset/49694/webkit#file9
41-
// FIXME: https://bugs.webkit.org/show_bug.cgi?id=212954
42-
static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesAnyFormOfGetPropertyNames | StructureIsImmortal;
36+
static constexpr unsigned StructureFlags = Base::StructureFlags | StructureIsImmortal;
4337

4438
template<typename CellType, SubspaceAccess mode>
4539
static IsoSubspace* subspaceFor(VM& vm)

0 commit comments

Comments
 (0)