1877193 - Make StaticPrefList.yaml prefs available in SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jan de Mooij [:jandem]]

Adding a new pref for a new feature or optimization currently requires a lot of boilerplate code. This is error-prone and we're spending quite a lot of time just adding/changing/removing/reviewing prefs.

I'm working on patches that eliminate almost all of this boilerplate by parsing StaticPrefList.yaml at SpiderMonkey build time and generating code to automatically read the javascript.options.* prefs.

To add a pref using this new system, all that is needed is:

1. Add the pref to StaticPrefList.yaml, for example javascript.options.amazing_new_feature.
2. Use JS::Prefs::amazing_new_feature() to retrieve its value in JS engine code.

Both JS shell and browser builds will default to the default value in StaticPrefList.yaml. In the JS shell, it'll be possible to set a pref value with --setpref amazing_new_feature=true or a custom shell flag or whatever.

At least initially this will only support process-wide prefs that are set at startup, but for 95% of our prefs that's all we need.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 1 - Make JS prefs in StaticPrefList.yaml available in SpiderMonkey. r?mgaudet!,KrisWright!

Adding a new pref for a new SpiderMonkey feature or optimization requires a lot of boilerplate code.

This patch stack lets us eliminate almost all of this boilerplate by parsing StaticPrefList.yaml
at SpiderMonkey build time and generating code to automatically expose the javascript.options.*
prefs that have the set_spidermonkey_pref attribute.

Both JS shell and browser builds will default to the pref value in StaticPrefList.yaml.
In the JS shell, it'll be possible to set a pref value with --setpref or a custom shell flag.
Code for this will be added in later patches.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 2 - Add JS shell flags to list and set prefs. r?mgaudet!

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 3 - Add testing functions to look up pref names and values. r?mgaudet!

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 4 - Use JS::Prefs for experimental.symbols_as_weakmap_keys pref. r?mgaudet!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 5 - Use JS::Prefs for array_grouping pref. r?mgaudet!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 6 - Use JS::Prefs for arraybuffer_transfer pref. r?mgaudet!

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 7 - Use JS::Prefs for well_formed_unicode_strings pref. r?allstarschh!

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 8 - Use JS::Prefs for property_error_message_fix pref. r?arai!

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 9 - Use JS::Prefs for experimental.iterator_helpers pref. r?dminor!

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 10 - Use JS::Prefs for experimental.shadow_realms pref. r?mgaudet!

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 11 - Use JS::Prefs for experimental.new_set_methods pref. r?anba!

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 12 - Use JS::Prefs for destructuring_fuse pref. r?iain!

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 13 - Use JS::Prefs for weakrefs prefs. r?jonco!

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 14 - Use JS::Prefs for use_fdlibm_for_sin_cos_tan pref. r?arai!

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 15 - Use JS::Prefs for site_based_pretenuring pref. r?jonco!

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 16 - Use JS::Prefs for resizable/growable arraybuffer prefs. r?anba!

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 17 - Add a jit-test. r?mgaudet!

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Bug 1877193 part 18 - Assert startup prefs are set before JS_Init*. r?mgaudet!

Looking at bug 1877605 made me realize we should define better when (startup) prefs
are set exactly.

This patch adds assertions to check startup prefs can only be set before JS_Init and
also fixes the embeddings to follow this new rule. This makes the JS shell and browser
behavior more consistent, and makes it possible to rely on pref values during startup.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eebff859fd3
part 1 - Make JS prefs in StaticPrefList.yaml available in SpiderMonkey. r=mgaudet,KrisWright
https://hg.mozilla.org/integration/autoland/rev/e233569c8754
part 2 - Add JS shell flags to list and set prefs. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/86209ac07283
part 3 - Add testing functions to look up pref names and values. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/9c895c695273
part 4 - Use JS::Prefs for experimental.symbols_as_weakmap_keys pref. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/dc1c78065ae2
part 5 - Use JS::Prefs for array_grouping pref. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/cb42ae6d0059
part 6 - Use JS::Prefs for arraybuffer_transfer pref. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/7cff18237238
part 7 - Use JS::Prefs for well_formed_unicode_strings pref. r=allstarschh
https://hg.mozilla.org/integration/autoland/rev/2fb2925094a6
part 8 - Use JS::Prefs for property_error_message_fix pref. r=arai
https://hg.mozilla.org/integration/autoland/rev/13d17ade20bf
part 9 - Use JS::Prefs for experimental.iterator_helpers pref. r=dminor
https://hg.mozilla.org/integration/autoland/rev/f51e25578cc2
part 10 - Use JS::Prefs for experimental.shadow_realms pref. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/e4e7a694f0ef
part 11 - Use JS::Prefs for experimental.new_set_methods pref. r=anba
https://hg.mozilla.org/integration/autoland/rev/aff2eb111a60
part 12 - Use JS::Prefs for destructuring_fuse pref. r=iain
https://hg.mozilla.org/integration/autoland/rev/ba71a363742f
part 13 - Use JS::Prefs for weakrefs prefs. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7dff3560bc3d
part 14 - Use JS::Prefs for use_fdlibm_for_sin_cos_tan pref. r=arai
https://hg.mozilla.org/integration/autoland/rev/16a3f8e4c145
part 15 - Use JS::Prefs for site_based_pretenuring pref. r=jonco
https://hg.mozilla.org/integration/autoland/rev/29d7680b19bd
part 16 - Use JS::Prefs for resizable/growable arraybuffer prefs. r=anba
https://hg.mozilla.org/integration/autoland/rev/9593f6918d71
part 17 - Add a jit-test. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/01cafce8c5d6
part 18 - Assert startup prefs are set before JS_Init*. r=mgaudet,necko-reviewers,valentin

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1eebff859fd3
https://hg.mozilla.org/mozilla-central/rev/e233569c8754
https://hg.mozilla.org/mozilla-central/rev/86209ac07283
https://hg.mozilla.org/mozilla-central/rev/9c895c695273
https://hg.mozilla.org/mozilla-central/rev/dc1c78065ae2
https://hg.mozilla.org/mozilla-central/rev/cb42ae6d0059
https://hg.mozilla.org/mozilla-central/rev/7cff18237238
https://hg.mozilla.org/mozilla-central/rev/2fb2925094a6
https://hg.mozilla.org/mozilla-central/rev/13d17ade20bf
https://hg.mozilla.org/mozilla-central/rev/f51e25578cc2
https://hg.mozilla.org/mozilla-central/rev/e4e7a694f0ef
https://hg.mozilla.org/mozilla-central/rev/aff2eb111a60
https://hg.mozilla.org/mozilla-central/rev/ba71a363742f
https://hg.mozilla.org/mozilla-central/rev/7dff3560bc3d
https://hg.mozilla.org/mozilla-central/rev/16a3f8e4c145
https://hg.mozilla.org/mozilla-central/rev/29d7680b19bd
https://hg.mozilla.org/mozilla-central/rev/9593f6918d71
https://hg.mozilla.org/mozilla-central/rev/01cafce8c5d6

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Jan, we're seeing pretty bing performance regression in DevTools tests (see https://treeherder.mozilla.org/perfherder/alerts?id=41399&hideDwnToInv=0 , mostly console tests), and this bug seems to be one of the potential offender in the push that triggered the regression (https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c48ca61bc9255db0107d377c103a752265b60eb8&tochange=aa11ea1d19a3fb9fccc3dd7cb02188d6109ec009). Does that sound plausible? Maybe the new setup means we're not updating a specific pref value or something?

Comment 22

[Jan de Mooij [:jandem]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #21)

Jan, we're seeing pretty bing performance regression in DevTools tests (see https://treeherder.mozilla.org/perfherder/alerts?id=41399&hideDwnToInv=0 , mostly console tests), and this bug seems to be one of the potential offender in the push that triggered the regression (https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c48ca61bc9255db0107d377c103a752265b60eb8&tochange=aa11ea1d19a3fb9fccc3dd7cb02188d6109ec009). Does that sound plausible? Maybe the new setup means we're not updating a specific pref value or something?

It's a bit unlikely because I don't think we flip any of these prefs in this case and they're not checked in performance sensitive code. We also haven't gotten other reports of performance regressions.

I triggered some extra damp runs on autoland, let's see if that tells us more. Unfortunately there was some build bustage right before this landed so the window includes some other changes too.

Comment 23

[Jan de Mooij [:jandem]]

Nicolas, the devtools performance regression has this range:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c48ca61bc9255db0107d377c103a752265b60eb8&tochange=e25850706df9ccdd4892444e83613dd37b57c4d1

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&series=autoland,4768146,1,12&timerange=1209600&zoom=1707146112307,1707300992384,950.9511034820435,1415.8686209746604

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan de Mooij [:jandem] from comment #23)

Nicolas, the devtools performance regression has this range:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c48ca61bc9255db0107d377c103a752265b60eb8&tochange=e25850706df9ccdd4892444e83613dd37b57c4d1

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&series=autoland,4768146,1,12&timerange=1209600&zoom=1707146112307,1707300992384,950.9511034820435,1415.8686209746604

Thanks a lot Jan