Bug 222518 – Reduce compile time and binary size cost of enabling proper CSSStyleDeclaration property access behavior

• Component: Bindings
• Status: RESOLVED FIXED

[Sam Weinig]

Once the change for https://bugs.webkit.org/show_bug.cgi?id=222517 is landed in it's disabled form, we need to work to remove the compile time and binary size regression that would come with enabling it.

[Sam Weinig]

A very simple idea, but with a runtime cost (though unlikely a runtime regression from the legacy code currently in the build, we should test) would be to replace all the getter / setters implementations with a single pair that re-lookup the name. Unfortunately, it seems only getters are passed their name in the callback, so at best (without making changes to JSC) we only be able to replace half the functions.

[Sam Weinig]

An alternative is that we could try to make the namedGetter/nameSetter approach work for this case by better emulating what a real property get/set would do for these properties.

[Sam Weinig]

If we don't go the "make it work like named getter/setter in getPropertySlot/putPropertySlot" route, I think the ideal would be a single function for all the gets, a single function for all the sets, and somehow encoding the CSSPropertyID in the static HashTableValues and passing that value to the shared functions. Been a long while since I looked at that part of JSC, so not sure how feasible that would be.

I'm a little confused about what role the static HashTables play these days. It seems like we mostly just use a const array of HashTableValues and call reifyStaticProperty on each one as soon as we create the prototype.

[Sam Sneddon [:gsnedders]]

(In reply to Sam Weinig from comment #1)
> A very simple idea, but with a runtime cost (though unlikely a runtime
> regression from the legacy code currently in the build, we should test)
> would be to replace all the getter / setters implementations with a single
> pair that re-lookup the name. Unfortunately, it seems only getters are
> passed their name in the callback, so at best (without making changes to
> JSC) we only be able to replace half the functions.

I'd be curious about why JSC has that asymmetry? At the spec level, neither getters nor setters are passed their names, so presumably there's some reason for this in JSC?

Also: how big is the binary size regression at this point? Is it just the generated getter/setter functions that dominate the regression?

[Sam Weinig]

Created attachment 422110 [details]
JSCSSStyleDeclaration.cpp (if you disable the legacy flag)

[Radar WebKit Bug Importer]

<rdar://problem/75136887>

[Sam Weinig]

(In reply to Sam Sneddon [:gsnedders] from comment #4)
> (In reply to Sam Weinig from comment #1)
> > A very simple idea, but with a runtime cost (though unlikely a runtime
> > regression from the legacy code currently in the build, we should test)
> > would be to replace all the getter / setters implementations with a single
> > pair that re-lookup the name. Unfortunately, it seems only getters are
> > passed their name in the callback, so at best (without making changes to
> > JSC) we only be able to replace half the functions.
> 
> I'd be curious about why JSC has that asymmetry? At the spec level, neither
> getters nor setters are passed their names, so presumably there's some
> reason for this in JSC?

I kind of doubt there is a good reason, probably just driven by needs since this is all internals stuff.

> 
> Also: how big is the binary size regression at this point? Is it just the
> generated getter/setter functions that dominate the regression?

Not sure at this point. It would be good to measure.


I have two ideas about to proceed here:

1) Add a PropertyName parameter to the custom setters, and implement a single set of getter/setters based on the property name parameter.

2) Add a mechanism to smuggle a parameter to the custom getter/setters stored that we store in the DOMAttributeGetterSetter at re-ification time during the prototype's creation. We could avoid making DOMAttributeGetterSetter bigger by utilizing the DOMAttributeAnnotation's domJIT pointer, which is always null in this case (and in most cases).


After sleeping on it, I'm somewhat leaning towards doing #1 first, just to see what if there is performance cost and if it fixes the compile time regression, as either solution is going to grow the number of parameters passed to custom setters by 1, and that scares me a bit.

[Sam Weinig]

I prototyped / implemented adding PropertyName to custom setters in https://bugs.webkit.org/show_bug.cgi?id=223413.

[Sam Weinig]

Now that setters have a property name, the next step is allowing multiple attributes to use the same function pointers for the getter/setter implementation. To do that, we need to fix bug 223542.

[Sam Weinig]

Or rather, we need to fix bug 223613.

[Sam Weinig]

Created attachment 425080 [details]
Patch

[Sam Weinig]

Created attachment 425105 [details]
Patch

[Sam Weinig]

Created attachment 425107 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #13)
> Created attachment 425107 [details]
> Patch

Blerg, looks like something is broken (or maybe has never worked) in the CMake builds that is causing JSCSSStyleDeclaration.cpp not to regenerate with the change in PlatformEnable.h.  I'll make an aggressive change in CSSStyleDeclaration.idl to try to force it.

[Sam Weinig]

Created attachment 425108 [details]
Patch

[Darin Adler]

Comment on attachment 425108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425108&action=review

r=me assuming tests pass (seeing EWS api-gtk failing right now?), there are not other scaling issues other than compile time and binary size, and this does resolve both of those issues as claimed

> Source/WebCore/bindings/js/JSDOMAttribute.h:61
> +    static bool setPassingPropertyName(JSC::JSGlobalObject& lexicalGlobalObject, JSC::EncodedJSValue thisValue, JSC::EncodedJSValue encodedValue, JSC::PropertyName attributeName)

Given overloading, can’t this be named set? Or do template arguments not do the overloading thing?

> LayoutTests/ChangeLog:14
> +        Remove tests for iteration order, which is not standardized, and not consistent among
> +        browsers.

But stability still seems desirable, so I’d kind of still wish for a test.

[Sam Weinig]

Created attachment 425158 [details]
Test overloading of non-type template parameters

[Sam Weinig]

(In reply to Darin Adler from comment #16)
> Comment on attachment 425108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425108&action=review
> 
> r=me assuming tests pass (seeing EWS api-gtk failing right now?), there are
> not other scaling issues other than compile time and binary size, and this
> does resolve both of those issues as claimed
> 
> > Source/WebCore/bindings/js/JSDOMAttribute.h:61
> > +    static bool setPassingPropertyName(JSC::JSGlobalObject& lexicalGlobalObject, JSC::EncodedJSValue thisValue, JSC::EncodedJSValue encodedValue, JSC::PropertyName attributeName)
> 
> Given overloading, can’t this be named set? Or do template arguments not do
> the overloading thing?

I didn't think you could overload using only non-type template parameters, but it seems you totally can. Will fix! (I uploaded my test for this since I thought it was interesting enough).

> 
> > LayoutTests/ChangeLog:14
> > +        Remove tests for iteration order, which is not standardized, and not consistent among
> > +        browsers.
> 
> But stability still seems desirable, so I’d kind of still wish for a test.

Yeah, I agree. Will make it a test that ensure the order by checking that we maintain the following invariants when iterating:

- 4 groupings
  - 1st grouping, camel case, alphabetical, includes properties with "Webkit" and "Epub" prefix with initial uppercase letter, no dashes anywhere in the name
  - 2nd grouping, webkit case, alphabetical, all properties prefixed with "webkit" with initial lowercase letter.
  - 3rd grouping, dashed case, alphabetical, all properties have a dash in them.
  - 4th grouping, epub case, alphabetical, all properties prefixed with "epub" with initial lowercase letter


Thanks for the review.

[Sam Weinig]

Created attachment 425187 [details]
Patch

[Sam Sneddon [:gsnedders]]

(In reply to Sam Weinig from comment #18)
> (In reply to Darin Adler from comment #16)
> > > LayoutTests/ChangeLog:14
> > > +        Remove tests for iteration order, which is not standardized, and not consistent among
> > > +        browsers.
> > 
> > But stability still seems desirable, so I’d kind of still wish for a test.
> 
> Yeah, I agree. Will make it a test that ensure the order by checking that we
> maintain the following invariants when iterating:
> 
> - 4 groupings
>   - 1st grouping, camel case, alphabetical, includes properties with
> "Webkit" and "Epub" prefix with initial uppercase letter, no dashes anywhere
> in the name
>   - 2nd grouping, webkit case, alphabetical, all properties prefixed with
> "webkit" with initial lowercase letter.
>   - 3rd grouping, dashed case, alphabetical, all properties have a dash in
> them.
>   - 4th grouping, epub case, alphabetical, all properties prefixed with
> "epub" with initial lowercase letter

Looking quickly, it seems that Firefox just iterates in the order they appear in the WebIDL file, which appears to be not really that well-defined (filesystem enumeration order defined!), but groups longhands, shorthands, and aliases together.

Not sure if I have any strong preference; I wonder if we could just get away with a single alphabetical ordering?

[Sam Weinig]

Created attachment 425687 [details]
Test Case

[Sam Weinig]

Created attachment 425697 [details]
Patch

[Darin Adler]

Now I feel bad suggesting you do the overloading, because it looks like it works fine on GCC and clang, but not the Microsoft compiler!

[Sam Weinig]

(In reply to Darin Adler from comment #23)
> Now I feel bad suggesting you do the overloading, because it looks like it
> works fine on GCC and clang, but not the Microsoft compiler!

Heh. But we both learned things! So win win!

[Sam Weinig]

Created attachment 425709 [details]
Patch

[EWS]

Committed r275808 (236379@main): <https://commits.webkit.org/236379@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425709 [details].