Summary: | [JSC] Simplify get*PropertyNames() methods and EnumerationMode | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alecflett, ashvayka, beidson, cdumez, ews-watchlist, hi, joepeck, jsbell, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=212956 https://bugs.webkit.org/show_bug.cgi?id=212909 |
||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 219926, 189034 | ||||||||||||
Attachments: |
|
Description
Mark Lam
2020-06-08 21:33:31 PDT
Ditto for JSGlobalObject. We should investigate whether the flag is really needed. Closed the wrong bug. Re-opening. Created attachment 416300 [details]
Patch
(In reply to Alexey Shvayka from comment #3) > Created attachment 416300 [details] > Patch r270393 patch object-get-own-property-symbols 64.6840+-0.9261 ^ 61.2901+-0.6377 ^ definitely 1.0554x faster object-keys-cloned-arguments 64.2298+-0.9984 ^ 52.0452+-1.3066 ^ definitely 1.2341x faster object-keys-error-object 392.5292+-9.7568 ^ 77.6232+-1.5012 ^ definitely 5.0569x faster Created attachment 416418 [details]
Patch
Fix early return condition in getNonReifiedStaticPropertyNames() and export getOwnSpecialPropertyNames().
Comment on attachment 416418 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416418&action=review r=me > Source/JavaScriptCore/runtime/JSObject.cpp:2384 > +constexpr unsigned maximumPrototypeChainDepth = 40000; Let's put `static ` before this. And maybe, defining it in JSObject's class static field (JSObject::maximumPrototypeChainDepth) is better. > Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:110 > + // Inlined JSObject::getOwnNonIndexPropertyNames() > + base->methodTable(vm)->getOwnSpecialPropertyNames(base, globalObject, propertyNames, DontEnumPropertiesMode::Exclude); > + RETURN_IF_EXCEPTION(scope, void()); > + > + base->getNonReifiedStaticPropertyNames(vm, propertyNames, DontEnumPropertiesMode::Exclude); > + unsigned nonStructurePropertyCount = propertyNames.size(); > + structure->getPropertyNamesFromStructure(vm, propertyNames, DontEnumPropertiesMode::Exclude); > + scope.assertNoException(); > + > + // |propertyNames| contains properties exclusively from the structure. > + if (!nonStructurePropertyCount) > + structurePropertyCount = propertyNames.size(); This means that, if non-reified property exists, we cannot use fast path (since nonStructurePropertyCount is non-zero, and structurePropertyCount is always zero). Is it the same in the old code? If the old code can use the fast path even if non-reified static property exists, can you change the new code to the same state? Created attachment 416886 [details]
Patch
Use JSObject::maximumPrototypeChainDepth and add a test case for non-reified static properties.
(In reply to Yusuke Suzuki from comment #6) > This means that, if non-reified property exists, we cannot use fast path > (since nonStructurePropertyCount is non-zero, and structurePropertyCount is > always zero). > Is it the same in the old code? If the old code can use the fast path even > if non-reified static property exists, can you change the new code to the > same state? Yes, right, this is very intentional improvement over old code: to match the order required by the spec, structure properties should come after non-reified static properties, which isn't possible on the fast path because structure loop comes before generic loop. This change is performance-neutral because slow path is taken only if there is an [[Enumerable]] non-reified or special property, which is extremely rare. In JSC, only `WebAssembly` and `WebAssembly.Module` objects have [[Enumerable]] non-reified properties. However, even they, as well as WebIDL interfaces like `Element.prototype`, never passed the canAccessPropertiesQuicklyForEnumeration() check because of accessors. I've added a test for for/in with `WebAssembly`, which used to fail before https://bugs.webkit.org/show_bug.cgi?id=214908. Created attachment 416888 [details]
Patch
Guard stress test with `WebAssembly` check.
Tools/Scripts/svn-apply failed to apply attachment 416888 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Committed r271269: <https://trac.webkit.org/changeset/271269> |