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.
Created attachment 280889 [details] WIP
Comment on attachment 280889 [details] WIP Attachment 280889 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1471224 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html streams/reference-implementation/pipe-through.html streams/brand-checks.html streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/count-queuing-strategy.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-2.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-1.html streams/readable-stream-reader-read.html streams/shadowing-Promise.html imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.https.html imported/w3c/web-platform-tests/streams/readable-streams/brand-checks.https.html imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.https.html streams/reference-implementation/bad-strategies.html imported/w3c/web-platform-tests/streams/readable-streams/cancel.https.html streams/reference-implementation/writable-stream.html streams/pipe-to.html streams/reference-implementation/bad-underlying-sinks.html streams/reference-implementation/pipe-to-options.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html streams/readable-stream-controller-error.html imported/w3c/web-platform-tests/streams/readable-streams/bad-strategies.https.html imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.https.html streams/reference-implementation/writable-stream-abort.html streams/reference-implementation/brand-checks.html
Created attachment 280897 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 280889 [details] WIP Attachment 280889 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1471229 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html streams/reference-implementation/pipe-through.html streams/brand-checks.html streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/count-queuing-strategy.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-2.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-1.html streams/readable-stream-reader-read.html streams/shadowing-Promise.html imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.https.html imported/w3c/web-platform-tests/streams/readable-streams/brand-checks.https.html imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.https.html streams/reference-implementation/bad-strategies.html imported/w3c/web-platform-tests/streams/readable-streams/cancel.https.html streams/reference-implementation/writable-stream.html streams/pipe-to.html streams/reference-implementation/bad-underlying-sinks.html streams/reference-implementation/pipe-to-options.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html streams/readable-stream-controller-error.html imported/w3c/web-platform-tests/streams/readable-streams/bad-strategies.https.html imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.https.html streams/reference-implementation/writable-stream-abort.html streams/reference-implementation/brand-checks.html
Created attachment 280899 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 280889 [details] WIP Attachment 280889 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1471177 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html streams/reference-implementation/pipe-through.html streams/brand-checks.html streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/count-queuing-strategy.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html streams/readable-stream-reader-read.html streams/shadowing-Promise.html imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.https.html imported/w3c/web-platform-tests/streams/readable-streams/brand-checks.https.html imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.https.html streams/reference-implementation/bad-strategies.html imported/w3c/web-platform-tests/streams/readable-streams/cancel.https.html streams/reference-implementation/writable-stream.html streams/pipe-to.html streams/reference-implementation/bad-underlying-sinks.html streams/reference-implementation/pipe-to-options.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html streams/readable-stream-controller-error.html imported/w3c/web-platform-tests/streams/readable-streams/bad-strategies.https.html imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.https.html streams/reference-implementation/writable-stream-abort.html streams/reference-implementation/brand-checks.html
Created attachment 280900 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 280889 [details] WIP Attachment 280889 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1471206 New failing tests: imported/w3c/web-platform-tests/streams/readable-streams/templated.https.html streams/reference-implementation/pipe-through.html streams/brand-checks.html streams/reference-implementation/readable-stream-templated.html streams/reference-implementation/count-queuing-strategy.html imported/w3c/web-platform-tests/streams/readable-streams/tee.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-2.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection-1.html streams/readable-stream-reader-read.html streams/shadowing-Promise.html imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.https.html imported/w3c/web-platform-tests/streams/readable-streams/brand-checks.https.html imported/w3c/web-platform-tests/fetch/api/response/response-cancel-stream.html imported/w3c/web-platform-tests/streams/readable-streams/readable-stream-reader.https.html streams/reference-implementation/bad-strategies.html imported/w3c/web-platform-tests/streams/readable-streams/cancel.https.html streams/reference-implementation/writable-stream.html streams/pipe-to.html streams/reference-implementation/bad-underlying-sinks.html streams/reference-implementation/pipe-to-options.html imported/w3c/web-platform-tests/streams/readable-streams/general.https.html imported/w3c/web-platform-tests/streams/readable-streams/garbage-collection.https.html streams/readable-stream-controller-error.html imported/w3c/web-platform-tests/streams/readable-streams/bad-strategies.https.html imported/w3c/web-platform-tests/streams/readable-streams/count-queuing-strategy-integration.https.html streams/reference-implementation/writable-stream-abort.html streams/reference-implementation/brand-checks.html
Created attachment 280902 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
WIP patch is good, but exposed latent bug tracked by bug #158637. Will update patch with ChangeLog & new bindings tests results once that lands.
Created attachment 281241 [details] Fix
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?
(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.
Created attachment 281625 [details] Updated fix
Created attachment 281626 [details] Updated fix
Comment on attachment 281626 [details] Updated fix This is so neat! r=me
Committed revision 202268.
Looks like a 49% regression on Dromaeo DOM Core on Mac?
Rolled out in <http://trac.webkit.org/changeset/202281> due to the huge Dromaeo DOM Core regression on Mac.
(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.
(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.
Created attachment 425041 [details] WIP
Created attachment 425632 [details] Patch
Created attachment 425633 [details] Microbenchmark
(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
Created attachment 425655 [details] Patch Define StructureFlags on DOM constructors merely as `const` to fix GTK build / Windows tests.
Awesome!
*** Bug 206837 has been marked as a duplicate of this bug. ***
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.
(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?
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.
(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!
Created attachment 428116 [details] Patch Merge hash table generation into GenerateHashTable and reengineer runtime enabled properties leveraging PropertyCallback.
(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.
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.
Haven't had time to look through the whole thing yet. Will get back to the rest soon.
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.
(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.
Looks good to me. Someone more familiar with the JSC side should probably do a pass as well though.
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?
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.
(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!
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.
Created attachment 432903 [details] Patch
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!
<rdar://problem/99527272>