Bug 158557

Summary: [WebIDL] Don't eagerly reify constructor and prototype properties
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: BindingsAssignee: Alexey Shvayka <ashvayka>
Status: REOPENED    
Severity: Normal CC: ahmad.saleem792, alecflett, ap, ashvayka, beidson, bfulgham, buildbot, cdumez, cgarcia, commit-queue, esprehn+autocc, ews-watchlist, ggaren, jsbell, keith_miller, kondapallykalyan, mark.lam, msaboff, nham, rniwa, saam, sam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 227275, 158637    
Bug Blocks:    
Attachments:
Description Flags
WIP
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews117 for mac-yosemite
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Fix
none
Updated fix
none
Updated fix
none
WIP
none
Patch
none
Microbenchmark
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch sam: review+, ews-feeder: commit-queue-

Gavin Barraclough
Reported 2016-06-09 00:04:44 PDT
The change to eagerly create these was made to avoid the overridden getOwnPropertySlot – however static tables no longer require this. Eager reification should no longer be the win it was, and comes at an initialization & memory cost.
Attachments
WIP (2.37 KB, patch)
2016-06-09 00:20 PDT, Gavin Barraclough 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-yosemite (1.80 MB, application/zip)
2016-06-09 01:35 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.54 MB, application/zip)
2016-06-09 01:38 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews117 for mac-yosemite (2.50 MB, application/zip)
2016-06-09 01:58 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1.27 MB, application/zip)
2016-06-09 02:14 PDT, Build Bot 		no flags
Details
Fix (66.32 KB, patch)
2016-06-14 00:26 PDT, Gavin Barraclough 		no flags
DetailsFormatted DiffDiff
Updated fix (66.37 KB, patch)
2016-06-19 17:09 PDT, Gavin Barraclough 		no flags
DetailsFormatted DiffDiff
Updated fix (67.01 KB, patch)
2016-06-19 17:11 PDT, Gavin Barraclough 		no flags
DetailsFormatted DiffDiff
WIP (32.82 KB, patch)
2021-04-02 12:31 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (433.16 KB, patch)
2021-04-09 10:47 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Microbenchmark (1.13 KB, text/html)
2021-04-09 10:49 PDT, Alexey Shvayka 		no flags
Details
Patch (440.92 KB, patch)
2021-04-09 14:51 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (718.72 KB, patch)
2021-05-09 01:20 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (732.35 KB, patch)
2021-05-10 07:02 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (812.30 KB, patch)
2021-06-21 09:22 PDT, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (74.79 KB, patch)
2021-07-05 16:26 PDT, Alexey Shvayka 		sam: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.
Gavin Barraclough
Comment 1 2016-06-09 00:20:17 PDT
Created attachment 280889 [details] WIP
Build Bot
Comment 2 2016-06-09 01:35:34 PDT Comment hidden (obsolete)
Build Bot
Comment 3 2016-06-09 01:35:38 PDT Comment hidden (obsolete)
Build Bot
Comment 4 2016-06-09 01:38:31 PDT Comment hidden (obsolete)
Build Bot
Comment 5 2016-06-09 01:38:35 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2016-06-09 01:58:53 PDT Comment hidden (obsolete)
Build Bot
Comment 7 2016-06-09 01:58:57 PDT Comment hidden (obsolete)
Build Bot
Comment 8 2016-06-09 02:14:05 PDT Comment hidden (obsolete)
Build Bot
Comment 9 2016-06-09 02:14:09 PDT Comment hidden (obsolete)
Gavin Barraclough
Comment 10 2016-06-10 18:15:24 PDT
WIP patch is good, but exposed latent bug tracked by bug #158637. Will update patch with ChangeLog & new bindings tests results once that lands.
Gavin Barraclough
Comment 11 2016-06-14 00:26:54 PDT
Created attachment 281241 [details] Fix
Geoffrey Garen
Comment 12 2016-06-14 10:46:31 PDT
Comment on attachment 281241 [details] Fix I think we're going to have a bad time if we reify one property at a time, since prototype watchpoints will then fire. How can we avoid changing these prototypes' structures? can we make them cacheable dictionaries from the start?
Gavin Barraclough
Comment 13 2016-06-17 00:00:46 PDT
(In reply to comment #12) > Comment on attachment 281241 [details] > Fix > > I think we're going to have a bad time if we reify one property at a time, > since prototype watchpoints will then fire. > > How can we avoid changing these prototypes' structures? can we make them > cacheable dictionaries from the start? Sounds like a sensible suggestion; previously we would have implicitly been doing so. Removing r?, will roll a new patch when I have a chance.
Gavin Barraclough
Comment 14 2016-06-19 17:09:11 PDT
Created attachment 281625 [details] Updated fix
Gavin Barraclough
Comment 15 2016-06-19 17:11:33 PDT
Created attachment 281626 [details] Updated fix
Andreas Kling
Comment 16 2016-06-20 11:09:18 PDT
Comment on attachment 281626 [details] Updated fix This is so neat! r=me
Gavin Barraclough
Comment 17 2016-06-20 21:56:51 PDT
Committed revision 202268.
Chris Dumez
Comment 18 2016-06-21 10:49:59 PDT
Looks like a 49% regression on Dromaeo DOM Core on Mac?
Chris Dumez
Comment 19 2016-06-21 10:57:53 PDT
Rolled out in <http://trac.webkit.org/changeset/202281> due to the huge Dromaeo DOM Core regression on Mac.
Chris Dumez
Comment 20 2016-06-21 10:58:40 PDT
(In reply to comment #19) > Rolled out in <http://trac.webkit.org/changeset/202281> due to the huge > Dromaeo DOM Core regression on Mac. Dromaeo JSLib and CSS Selector tests were also significantly regressed.
Chris Dumez
Comment 21 2016-06-21 18:44:57 PDT
(In reply to comment #20) > (In reply to comment #19) > > Rolled out in <http://trac.webkit.org/changeset/202281> due to the huge > > Dromaeo DOM Core regression on Mac. > > Dromaeo JSLib and CSS Selector tests were also significantly regressed. Dromaeo recovered after the roll out.
Alexey Shvayka
Comment 22 2021-04-02 12:31:24 PDT
Created attachment 425041 [details] WIP
Alexey Shvayka
Comment 23 2021-04-09 10:47:09 PDT
Created attachment 425632 [details] Patch
Alexey Shvayka
Comment 24 2021-04-09 10:49:20 PDT
Created attachment 425633 [details] Microbenchmark
Alexey Shvayka
Comment 25 2021-04-09 10:49:40 PDT
(In reply to Alexey Shvayka from comment #23) > Created attachment 425632 [details] > Patch Tools/Scripts/run-perf-tests --release --no-build PerformanceTests/Dromaeo (4 runs with 400s intervals) subtest trunk (runs/s) patch (runs/s) dom-attr 455.67 487.44 ? might be 7.0% faster dom-modify 99.47 104.04 ? might be 4.6% faster dom-query 1045.20 1130.88 ? might be 8.2% faster dom-traverse 228.41 232.65 dromaeo-3d-cube 4943.50 4954.25 dromaeo-core-eval 123.16 124.67 dromaeo-object-array 25.80 25.76 dromaeo-object-regexp 4.0610 4.0184 dromaeo-object-string 122.42 120.55 dromaeo-string-base64 1663.31 1661.15 jslib-attr-jquery 520.36 526.97 jslib-attr-prototype 1088.92 1104.18 jslib-event-jquery 0.098819 0.098925 jslib-event-prototype 1275.85 1262.84 jslib-style-jquery 130.99 130.47 jslib-style-prototype 251.52 245.35 ? might be 2.5% slower jslib-traverse-jquery 45.97 45.43 jslib-traverse-prototype 662.08 651.61
Alexey Shvayka
Comment 26 2021-04-09 14:51:47 PDT
Created attachment 425655 [details] Patch Define StructureFlags on DOM constructors merely as `const` to fix GTK build / Windows tests.
Sam Weinig
Comment 27 2021-04-10 08:22:51 PDT
Awesome!
Sam Weinig
Comment 28 2021-04-11 09:09:38 PDT
*** Bug 206837 has been marked as a duplicate of this bug. ***
Sam Weinig
Comment 29 2021-04-11 09:28:59 PDT
Comment on attachment 425655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425655&action=review > Source/JavaScriptCore/ChangeLog:11 > + 2. Removes invariant that DOMJITAttribute property is read-only, that was broken by > + `document.body` setter, aligning non-reified properties with reifyStaticProperty(). Is DOMJITAttribute generally usable for setters? Is https://bugs.webkit.org/show_bug.cgi?id=172700 something we can now look into doing (would be a really nice code size reduction)? > Source/JavaScriptCore/runtime/Lookup.h:230 > + if (entry->attributes() & PropertyAttribute::BuiltinOrFunctionOrAccessorOrLazyPropertyOrConstant) > + return setUpStaticPropertySlot(vm, classInfo, entry, thisObject, propertyName, slot); Unrelated to this patch, but I wonder if this function, getStaticPropertySlotFromTable, is really worth inlining at this size (or, if it would make more sense to just move it JSObject.cpp since that seems to be the only caller now). > Source/WebCore/ChangeLog:25 > + This change increases WebCore --release binary size by 0.34% (255 KB) due to index > + tables being generated for faster static hash table lookups. This seems acceptable, especially since we have had some binary reduction patches in this area recently, but I wonder if we can make this a bit smaller if we templatized JSC::CompactHashIndex and JSC::HashTable to allow JSC::CompactHashIndex to use int8_t sized values rather than int16_t when that would be big enough. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7202 > - push(@implContent, "\nstatic const HashTableValue $nameEntries\[\] =\n\{\n"); > + push(@implContent, "\nstatic HashTableValue $nameEntries\[\] =\n\{\n"); Can we keep this const in the case that there are no runtime runtime/settings enabled features? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:8007 > + push(@$outputArray, " ${hashTableValuesName}[${index}].m_key = ${runtimeEnableConditionalString} ? \"${name}\" : nullptr;\n"); I think this will break things if two pages in the same process have different Settings, since this modifies the global table used by both.
Sam Weinig
Comment 30 2021-04-27 08:48:27 PDT
(In reply to Sam Weinig from comment #29) > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:8007 > > + push(@$outputArray, " ${hashTableValuesName}[${index}].m_key = ${runtimeEnableConditionalString} ? \"${name}\" : nullptr;\n"); > > I think this will break things if two pages in the same process have > different Settings, since this modifies the global table used by both. Since I don't have a good solution to this problem, perhaps we cold change this to avoid eager reify for any type that does not have settings/runtime changeable properties?
Alexey Shvayka
Comment 31 2021-04-27 09:27:59 PDT
Comment on attachment 425655 [details] Patch (In reply to Sam Weinig from comment #30) > (In reply to Sam Weinig from comment #29) > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:8007 > > > + push(@$outputArray, " ${hashTableValuesName}[${index}].m_key = ${runtimeEnableConditionalString} ? \"${name}\" : nullptr;\n"); > > > > I think this will break things if two pages in the same process have > > different Settings, since this modifies the global table used by both. > > Since I don't have a good solution to this problem, perhaps we cold change > this to avoid eager reify for any type that does not have settings/runtime > changeable properties? I've figured out a great solution: to use PropertyAttribute::PropertyCallback like we do for JSGlobalObject. It will allow us to preserve immutability of static hash tables without adding extra checks / fields on them, and is also a nice refactor around GenerateHashTable. Currently on ChangeLog stage, will upload the patch soon enough.
Sam Weinig
Comment 32 2021-05-07 09:14:04 PDT
(In reply to Alexey Shvayka from comment #31) > Comment on attachment 425655 [details] > Patch > > (In reply to Sam Weinig from comment #30) > > (In reply to Sam Weinig from comment #29) > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:8007 > > > > + push(@$outputArray, " ${hashTableValuesName}[${index}].m_key = ${runtimeEnableConditionalString} ? \"${name}\" : nullptr;\n"); > > > > > > I think this will break things if two pages in the same process have > > > different Settings, since this modifies the global table used by both. > > > > Since I don't have a good solution to this problem, perhaps we cold change > > this to avoid eager reify for any type that does not have settings/runtime > > changeable properties? > > I've figured out a great solution: to use > PropertyAttribute::PropertyCallback like we do for JSGlobalObject. > It will allow us to preserve immutability of static hash tables without > adding extra checks / fields on them, and is also a nice refactor around > GenerateHashTable. > Currently on ChangeLog stage, will upload the patch soon enough. Very cool!
Alexey Shvayka
Comment 33 2021-05-09 01:20:07 PDT
Created attachment 428116 [details] Patch Merge hash table generation into GenerateHashTable and reengineer runtime enabled properties leveraging PropertyCallback.
Alexey Shvayka
Comment 34 2021-05-09 01:20:24 PDT
(In reply to Sam Weinig from comment #29) Thank you for super valuable insights! > > Source/JavaScriptCore/ChangeLog:11 > > + 2. Removes invariant that DOMJITAttribute property is read-only, that was broken by > > + `document.body` setter, aligning non-reified properties with reifyStaticProperty(). > > Is DOMJITAttribute generally usable for setters? Not currently. `document.body` has a DOMJIT getter and regular non-JIT setter. Clarified the ChangeLog. > Is https://bugs.webkit.org/show_bug.cgi?id=172700 something we can now look into doing (would be a really nice code size reduction)? Yeah, that would be nice to have. JSObject::putInlineSlow() was recently reworked, which will probably simplify implementation. > > Source/JavaScriptCore/runtime/Lookup.h:230 > > + if (entry->attributes() & PropertyAttribute::BuiltinOrFunctionOrAccessorOrLazyPropertyOrConstant) > > + return setUpStaticPropertySlot(vm, classInfo, entry, thisObject, propertyName, slot); > > Unrelated to this patch, but I wonder if this function, getStaticPropertySlotFromTable, is really worth inlining > at this size (or, if it would make more sense to just move it JSObject.cpp since that seems to be the only caller now). Currently this function checks staticPropertiesReified() while being called within a loop. We should definitely inline it into getOwnStaticPropertySlot(), hoisting the check. Added a FIXME. > > Source/WebCore/ChangeLog:25 > > + This change increases WebCore --release binary size by 0.34% (255 KB) due to index > > + tables being generated for faster static hash table lookups. > > This seems acceptable, especially since we have had some binary reduction patches in this area recently, Yeah, WebCore binary was recently shrunk by ~10 MB; not sure which patch did that. I'm certain that https://bugs.webkit.org/show_bug.cgi?id=223758 will soon regain the increase. > I wonder if we can make this a bit smaller if we templatized JSC::CompactHashIndex and JSC::HashTable > to allow JSC::CompactHashIndex to use int8_t sized values rather than int16_t when that would be big enough. Added a FIXME since the patch is quite large and that's non-trivial change. Also, with int8_t, we should try to utilize all the integer range, rather than -1 .. 127. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7202 > > - push(@implContent, "\nstatic const HashTableValue $nameEntries\[\] =\n\{\n"); > > + push(@implContent, "\nstatic HashTableValue $nameEntries\[\] =\n\{\n"); > > Can we keep this const in the case that there are no runtime runtime/settings enabled features? New approach preserves immutability. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:8007 > > + push(@$outputArray, " ${hashTableValuesName}[${index}].m_key = ${runtimeEnableConditionalString} ? \"${name}\" : nullptr;\n"); > > I think this will break things if two pages in the same process have > different Settings, since this modifies the global table used by both. I've missed that very obvious detail; not so many settings within the (open source part) of WebKit seem be set per-realm though.
Sam Weinig
Comment 35 2021-05-09 10:20:17 PDT
Comment on attachment 428116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428116&action=review > Source/WebCore/ChangeLog:25 > + Also, this change removes all usages of DeletePropertyModeScope from WebCore, which was > + used for runtime disabled non-configurable constants, allowing its removal from JSC. Did you actually remove DeletePropertyModeScope from JSC, or are you planning to do that in a future change? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3290 > + assert("Using DOMJIT with runtime enabled attributes and private identifiers is not yet supported.") if $attribute->extendedAttributes->{DOMJIT}; This assert seems a bit odd at least with the current name of this subroutine, as it is not immediately clear what this has to do with runtime enabled attributes or private identifiers. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3302 > + assert("Using DOMJIT with runtime enabled operations and private identifiers is not yet supported.") if $operation->extendedAttributes->{DOMJIT}; This assert seems a bit odd at least with the current name of this subroutine, as it is not immediately clear what this has to do with runtime enabled attributes or private identifiers. > Source/WebCore/bindings/scripts/IDLParser.pm:83 > +sub IDLInterface::className > +{ > + my $self = shift; > + return "JS" . $self->type->name; > +} I've tried to keep JS specific stuff outside of IDLParser in the past. Think this should stay in CodeGeneratorJS.
Sam Weinig
Comment 36 2021-05-09 10:22:57 PDT
Haven't had time to look through the whole thing yet. Will get back to the rest soon.
Alexey Shvayka
Comment 37 2021-05-10 07:02:39 PDT
Created attachment 428167 [details] Patch Reify runtime enabled properties in putInlineSlow(), add bindings tests for DOM attributes with private identifiers, add static_cast for IsEnabled callbacks, clarify DOMJIT assertions and ChangeLog, and move IDLInterface::className to CodeGeneratorJS.
Alexey Shvayka
Comment 38 2021-05-10 07:08:05 PDT
(In reply to Sam Weinig from comment #35) > Comment on attachment 428116 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428116&action=review > > > Source/WebCore/ChangeLog:25 > > + Also, this change removes all usages of DeletePropertyModeScope from WebCore, which was > > + used for runtime disabled non-configurable constants, allowing its removal from JSC. > > Did you actually remove DeletePropertyModeScope from JSC, or are you > planning to do that in a future change? Not yet: JSC has one last usage for global variables; clarified the ChangeLog. I am overly excited about this because DeletePropertyModeScope breaks invariants of the internal methods. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3290 > > + assert("Using DOMJIT with runtime enabled attributes and private identifiers is not yet supported.") if $attribute->extendedAttributes->{DOMJIT}; > > This assert seems a bit odd at least with the current name of this > subroutine, as it is not immediately clear what this has to do with runtime > enabled attributes or private identifiers. Fixed. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3302 > > + assert("Using DOMJIT with runtime enabled operations and private identifiers is not yet supported.") if $operation->extendedAttributes->{DOMJIT}; > > This assert seems a bit odd at least with the current name of this > subroutine, as it is not immediately clear what this has to do with runtime > enabled attributes or private identifiers. Fixed. > > Source/WebCore/bindings/scripts/IDLParser.pm:83 > > +sub IDLInterface::className > > +{ > > + my $self = shift; > > + return "JS" . $self->type->name; > > +} > > I've tried to keep JS specific stuff outside of IDLParser in the past. Think > this should stay in CodeGeneratorJS. Nice, moved to CodeGeneratorJS.pm.
Sam Weinig
Comment 39 2021-05-10 10:20:21 PDT
Looks good to me. Someone more familiar with the JSC side should probably do a pass as well though.
Saam Barati
Comment 40 2021-05-17 13:00:53 PDT
Comment on attachment 428167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428167&action=review LGTM, just a few comments. > Source/JavaScriptCore/ChangeLog:16 > + 2. Enables LazyPropertyCallback to return GetterSetter / CustomGetterSetter, ensuring > + correct structure flags and slot initialization. What was it used for before? Might be worth stating that here > Source/JavaScriptCore/runtime/JSObject.cpp:795 > + slot.disableCaching(); I wonder if long term we could have a way of telling the IC to look at the new result from the structure lookup on the LOC below this. Right now, if we see a different structure, we might assume a Transition. Maybe we could find a way to tell the IC to ignore this reification just once. Might not be important now, but maybe longer term, it would be a nice thing to do. > Source/JavaScriptCore/runtime/JSObject.cpp:796 > + return lookupPropertyForPut<LookupKind::StructureOnly>(vm, object, object->structure(vm), propertyName, slot); nit: I think a nicer way to do this would be to just make the structure lookup a lambda. Do away with this LookupKind enum. The main caller of this function shouldn't worry about this implementation detail. Then here, instead of recursing with a different enum, you can just return the invocation of the structure lookup lambda. > Source/JavaScriptCore/runtime/JSObject.cpp:798 > + // FIXME: Remove this after writable accessors are introduced to static hash tables. not sure we really need this FIXME anymore > Source/JavaScriptCore/runtime/Lookup.h:41 > +// FIXME: Templatize HashTable and CompactHashIndex so int8_t sized values can be used for small tables to reduce binary size. link a bugzilla > Source/JavaScriptCore/runtime/Lookup.h:109 > + auto isEnabledCallback = reinterpret_cast<IsLazyPropertyEnabledCallback>(m_values.value2); bitwise_cast > Source/WebCore/ChangeLog:22 > + Another important advantage: a feature that was enabled after its interface was created, > + immediately becomes usable (no page reload needed). except for inline caching?
Alexey Shvayka
Comment 41 2021-06-21 09:22:59 PDT
Created attachment 431869 [details] Patch Account for Saam's feedback, fix ApplePay test, rename to introduce private identifiers for static attributes / operations, don't mix other property attributes with PropertyCallback, remove extra checks from setUpStaticPropertySlot(), tweak IC to check for isTaintedByOpaqueObject() and add thorough test coverage for it, revert not defaulting to CustomValue, introduce StringifyJSCAttributes.
Alexey Shvayka
Comment 42 2021-06-21 09:23:43 PDT
(In reply to Saam Barati from comment #40) Thank you for review, Saam! It's very helpful. > > Source/JavaScriptCore/ChangeLog:16 > > + 2. Enables LazyPropertyCallback to return GetterSetter / CustomGetterSetter, ensuring > > + correct structure flags and slot initialization. > > What was it used for before? Might be worth stating that here Before, it was used only to putDirect() objects (Reflect, Math) and constructors (Intl.Collator, WebAssembly.Table). Updated the ChangeLog. > > Source/JavaScriptCore/runtime/JSObject.cpp:795 > > + slot.disableCaching(); > > I wonder if long term we could have a way of telling the IC to look at the > new result from the structure lookup on the LOC below this. Right now, if we > see a different structure, we might assume a Transition. Maybe we could find > a way to tell the IC to ignore this reification just once. Might not be > important now, but maybe longer term, it would be a nice thing to do. Yeah, it would be nice for IC to kick in a little earlier. Let's revisit this after reification is fixed not to trigger structure transitions / to dictionary conversion as Filip suggested during the call. > > Source/JavaScriptCore/runtime/JSObject.cpp:796 > > + return lookupPropertyForPut<LookupKind::StructureOnly>(vm, object, object->structure(vm), propertyName, slot); > > nit: > > I think a nicer way to do this would be to just make the structure lookup a > lambda. Do away with this LookupKind enum. The main caller of this function > shouldn't worry about this implementation detail. > > Then here, instead of recursing with a different enum, you can just return > the invocation of the structure lookup lambda. Changed. Nice point on LookupKind being an unnecessary implementation detail. > > Source/JavaScriptCore/runtime/JSObject.cpp:798 > > + // FIXME: Remove this after writable accessors are introduced to static hash tables. > > not sure we really need this FIXME anymore Ah, this is still an issue: unlike WebIDL code generator, JSC's create_hash_table doesn't set ReadOnly for setter-less accessors. GetterSetter constructor gracefully handles that (via NullSetterFunction), but putInlineSlow() doesn't support non-reified Accessor properties. I've reworded the FIXME, since I no longer consider that writable accessors are necessary to implement legacy RegExp features (https://bugs.webkit.org/show_bug.cgi?id=220233). Instead, custom accessors should pass correct global object (https://bugs.webkit.org/show_bug.cgi?id=225997), and create_hash_table is being fixed as part of that change. > > Source/JavaScriptCore/runtime/Lookup.h:41 > > +// FIXME: Templatize HashTable and CompactHashIndex so int8_t sized values can be used for small tables to reduce binary size. > > link a bugzilla Added: https://bugs.webkit.org/show_bug.cgi?id=227216. > > Source/JavaScriptCore/runtime/Lookup.h:109 > > + auto isEnabledCallback = reinterpret_cast<IsLazyPropertyEnabledCallback>(m_values.value2); > > bitwise_cast Fixed. > > Source/WebCore/ChangeLog:22 > > + Another important advantage: a feature that was enabled after its interface was created, > > + immediately becomes usable (no page reload needed). > > except for inline caching? I was intentially calling disableCache() even if reifyStaticProperty() didn't produce a property for runtime disabled field. However, I've totally missed [[Get]] & hasOwnProperty() caching, which is now fixed and thoroughly covered by tests. Great catch!
Alexey Shvayka
Comment 43 2021-06-22 19:16:37 PDT
LayoutTests/webgl/2.0.0/conformance/state/gl-object-get-calls.html --debug timeouts are not random; in --release, this patch regresses from 37s to 52s (100 runs). I've tracked that down to wtu.glEnumToString() performing for/in on WebGLRenderingContext's prototype, reifing most of the properties and making the holder a cacheable dictionary. The next reification makes property lookups uncacheable because the dictionary was already flattened in prepareChainForCaching(), slowing down the hot loop in testInvalidArgument(). The slowdown is regained if all properties of WebGLRenderingContext's prototype are reified at once, via `delete` or Reflect.ownKeys + get(). Althought we may consider passing a flag in hasEnumerableProperty() to avoid reification, even without the for/in, this test regresses after 64th reification. Filip suggested that instead of reifing via putDirect(), we nuke the previous transition (if it's reification as well), to avoid transitioning to cacheable dictionary in the first place. Something similar to putDirectWithoutTransition() I guess? Since most of this patch is a refactor of bindings generator and there is a good chance we encounter other regressions, I'm landing this change with eager property reification (performance-neutral) in https://bugs.webkit.org/show_bug.cgi?id=227275 to avoid future merge conflicts and to unblock a few other patches that kinda depend on it.
Alexey Shvayka
Comment 44 2021-07-05 16:26:11 PDT
Created attachment 432903 [details] Patch
Ahmad Saleem
Comment 45 2022-09-03 05:00:00 PDT
Micro bench updated test results: *** Safari Technology Preview 152 *** AVG - 25.xx to 27.xx ms Lowest - 17ms *** Chrome Canary 107 *** AVG - with enough runs, it goes into 0.27 ms and continue decreasing. First run - 10 ms and then subsequent runs 0 ms *** Firefox Nightly 106 *** AVG - 5.xx ms and hove from 7 ms to 4 ms if continues. _______ Just wanted to share updated results for microbenchmark and mention that this bug has r+ patch. Thanks!
Radar WebKit Bug Importer
Comment 46 2022-09-03 09:53:27 PDT
<rdar://problem/99527272>
Note You need to log in before you can comment on or make changes to this bug.