1490044 - Use StaticPrefs directly in WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

StaticPrefs are nice because they are (if marked as atomic) thread-safe.
Currently, for many APIs, we use [Func="foo"] just to check a pref, in thread-safe way. DOMPrefs.h does this for several DOM preferences.

Would be nice to have [StaticPref="foo"] attribute in our WebIDL parser/code generator and get rid of DOMPrefs.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - [StaticPref="foo"]

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - DOMPrefs

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Remove DOMPrefs

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 9007807 [details] [diff] [review]
part 1 - [StaticPref="foo"]

Review of attachment 9007807 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +1229,5 @@
>              # getExtendedAttribute() returns a list, extract the entry.
>              funcList = desc.interface.getExtendedAttribute("Func")
>              if funcList is not None:
>                  addHeaderForFunc(funcList[0], desc)
> +            # getExtendedAttribute() returns a list, extract the entry.

Drop the comment, you're not extracting the entry.

@@ +2145,5 @@
>                       prefCacheTemplate % (name, len(prefableSpecs))))
> +            # StaticPref converted to method call.
> +            staticPrefMethod = "nullptr";
> +            if condition.staticPref is not None:
> +                staticPrefMethod = "StaticPrefs::%s" % condition.staticPref.replace(".", "_")

This will need to be clearly documented (replacing . with _), because afaict users of the VARCACHE_PREF macro can choose to use whatever id they want.

::: dom/bindings/DOMJSClass.h
@@ +145,5 @@
>    // because it will change at runtime if the corresponding pref is changed.
>    bool enabled;
>  
> +  // Static pref method.
> +  bool (*staticPref)();

Can we make this a const pointer?

Comment 5

[Peter Van der Beken [:peterv]]

Bz and I talked a bit about this, and he raised some concerns:

1) The overhead of calling the function call
2) The binary size hit of the function pointer (made worse by the unfortunate positioning of the pointer in between bools)
3) More relocations

1 and 3 are not problems in particular with the ones you convert here (since they used Func before), but 2 is. However, if we plan to move everything to StaticPrefs in the future, then it might make more sense to do the whole conversion at once here.

For the binary size hit, bz proposed making an array of function pointers to StaticPrefs' functions and storing an index in PrefableDisablers. If we take some bits from nonExposedGlobals and secureContext (which can use 1 bit), and drop the |enabled| bool, we have plenty of space to store such an index. The hope is that the array of function pointers will be less of a size hit and require fewer relocations. Also, because we remove the non-const |enabled| bool we can make the PrefableDisablers arrays const.

After talking to bz I think I agree with his concerns. Would you be up for doing these changes? I can try to take over if not.

Comment 6

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9890a72bda40f64c9dccb45eebd56d52a13c5fa

Comment 7

[Andrea Marchesini [:baku]]

After talking to bz I think I agree with his concerns. Would you be up for
doing these changes? I can try to take over if not.

Peter, I'll not be able to work on this. Do you have cycles?

Comment 8

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=880e9834e123394d01efd4612805cd92cf955b28

Comment 9

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6609cb5a8d78d295c5e19baaf34308f09a4912c

Comment 10

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c246361ae7c2a7e7ace4cf156dd09bc573a20835

Comment 11

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=325169fd6a56337439b2d59167da9cbbb24309fc

Comment 12

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9661dc84a7df9a1985e8ba8a4f17c05aafb520e

Comment 13

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46591314230b455c3480b2909d7312ef89fdaca2

Comment 14

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=164e442f4595319a492e66bf0aaef84f478d8947

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Bug 1490044 - Move all prefs used in WebIDL to StaticPrefs. r?bzbarsky!

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Bug 1490044 - Convert WebIDL pref disablers to use StaticPrefs. r?bzbarsky!

Depends on D33507

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Bug 1490044 - Remove DOMPrefs MACRO for WebIDL support. r?bzbarsky!

Depends on D33508

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Bug 1490044 - Remove DOMPrefs. r?bzbarsky!

Depends on D33509

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1091aed2035c
Move all prefs used in WebIDL to StaticPrefs. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/7526d2cbe12d
Convert WebIDL pref disablers to use StaticPrefs. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/f5f71d6a5abc
Remove DOMPrefs MACRO for WebIDL support. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/3e6dc353ad6e
Remove DOMPrefs. r=bzbarsky

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1091aed2035c
https://hg.mozilla.org/mozilla-central/rev/7526d2cbe12d
https://hg.mozilla.org/mozilla-central/rev/f5f71d6a5abc
https://hg.mozilla.org/mozilla-central/rev/3e6dc353ad6e

Comment 21

[u636583]

uhm,
0:03.54 In file included from /home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/dom/bindings/UnifiedBindings1.cpp:2:
0:03.54 /home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/dom/bindings/AudioWorkletBinding.cpp:4:10: fatal error: 'DOMPrefs.h' file not found
0:03.54 #include "DOMPrefs.h"
0:03.54 ^~~~~~~~~~~~
0:04.40 In file included from /home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/dom/bindings/UnifiedBindings0.cpp:2:
0:04.40 /home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/dom/bindings/APZTestDataBinding.cpp:5:10: fatal error: 'DOMPrefs.h' file not found
0:04.40 #include "DOMPrefs.h"
0:04.40 ^~~~~~~~~~~~

tested on:
changeset: 478901:b3a69dff7e25
tag: tip
date: Thu Jun 13 20:21:46 2019 +0000
summary: Bug 1558485 - Turn on ESLint for all of dom/ disabling most of the failing rules. r=smaug

Comment 22

[u636583]

Looks like it comes from:
dom/bindings/Codegen.py:14705: bindingHeaders["DOMPrefs.h"] = descriptors

Comment 23

[u636583]

Attachment: le.patch

yep that fixed it(aka removed all 124 includes from obj-x86_64-pc-linux-gnu/dom/bindings/)
(now, it's still compiling so unsure if it still finishes successfully, but assuming it will;-) )

Comment 24

[u636583]

ok my bad, turns out I was already apply a patch earlier which added it! hence why removing it was needed:

from https://github.com/bn0785ac/firefox-beta/blob/master/fix.patch
via https://bugzilla.mozilla.org/show_bug.cgi?id=1433562#c3