Bug 158557 - [WebIDL] Don't eagerly reify constructor and prototype properties
Summary: [WebIDL] Don't eagerly reify constructor and prototype properties
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
: 206837 (view as bug list)
Depends on: 227275 158637
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-09 00:04 PDT by Gavin Barraclough
Modified: 2022-09-04 13:24 PDT (History)
25 users (show)

See Also:


Attachments
WIP (2.37 KB, patch)
2016-06-09 00:20 PDT, Gavin Barraclough
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Updated fix (66.37 KB, patch)
2016-06-19 17:09 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Updated fix (67.01 KB, patch)
2016-06-19 17:11 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
WIP (32.82 KB, patch)
2021-04-02 12:31 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (433.16 KB, patch)
2021-04-09 10:47 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (718.72 KB, patch)
2021-05-09 01:20 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (732.35 KB, patch)
2021-05-10 07:02 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (812.30 KB, patch)
2021-06-21 09:22 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (74.79 KB, patch)
2021-07-05 16:26 PDT, Alexey Shvayka
sam: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 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.
Comment 1 Gavin Barraclough 2016-06-09 00:20:17 PDT
Created attachment 280889 [details]
WIP
Comment 2 Build Bot 2016-06-09 01:35:34 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2016-06-09 01:35:38 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2016-06-09 01:38:31 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2016-06-09 01:38:35 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2016-06-09 01:58:53 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2016-06-09 01:58:57 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2016-06-09 02:14:05 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2016-06-09 02:14:09 PDT Comment hidden (obsolete)
Comment 10 Gavin Barraclough 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.
Comment 11 Gavin Barraclough 2016-06-14 00:26:54 PDT
Created attachment 281241 [details]
Fix
Comment 12 Geoffrey Garen 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?
Comment 13 Gavin Barraclough 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.
Comment 14 Gavin Barraclough 2016-06-19 17:09:11 PDT
Created attachment 281625 [details]
Updated fix
Comment 15 Gavin Barraclough 2016-06-19 17:11:33 PDT
Created attachment 281626 [details]
Updated fix
Comment 16 Andreas Kling 2016-06-20 11:09:18 PDT
Comment on attachment 281626 [details]
Updated fix

This is so neat! r=me
Comment 17 Gavin Barraclough 2016-06-20 21:56:51 PDT
Committed revision 202268.
Comment 18 Chris Dumez 2016-06-21 10:49:59 PDT
Looks like a 49% regression on Dromaeo DOM Core on Mac?
Comment 19 Chris Dumez 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.
Comment 20 Chris Dumez 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.
Comment 21 Chris Dumez 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.
Comment 22 Alexey Shvayka 2021-04-02 12:31:24 PDT
Created attachment 425041 [details]
WIP
Comment 23 Alexey Shvayka 2021-04-09 10:47:09 PDT
Created attachment 425632 [details]
Patch
Comment 24 Alexey Shvayka 2021-04-09 10:49:20 PDT
Created attachment 425633 [details]
Microbenchmark
Comment 25 Alexey Shvayka 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
Comment 26 Alexey Shvayka 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.
Comment 27 Sam Weinig 2021-04-10 08:22:51 PDT
Awesome!
Comment 28 Sam Weinig 2021-04-11 09:09:38 PDT
*** Bug 206837 has been marked as a duplicate of this bug. ***
Comment 29 Sam Weinig 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.
Comment 30 Sam Weinig 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?
Comment 31 Alexey Shvayka 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.
Comment 32 Sam Weinig 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!
Comment 33 Alexey Shvayka 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.
Comment 34 Alexey Shvayka 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.
Comment 35 Sam Weinig 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.
Comment 36 Sam Weinig 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.
Comment 37 Alexey Shvayka 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.
Comment 38 Alexey Shvayka 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.
Comment 39 Sam Weinig 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.
Comment 40 Saam Barati 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?
Comment 41 Alexey Shvayka 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.
Comment 42 Alexey Shvayka 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!
Comment 43 Alexey Shvayka 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.
Comment 44 Alexey Shvayka 2021-07-05 16:26:11 PDT
Created attachment 432903 [details]
Patch
Comment 45 Ahmad Saleem 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!
Comment 46 Radar WebKit Bug Importer 2022-09-03 09:53:27 PDT
<rdar://problem/99527272>