Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#8539 Config preferences backend restructure #22923

Merged
merged 1 commit into from Mar 20, 2019

Conversation

Projects
None yet
8 participants
@peterjoel
Copy link
Contributor

peterjoel commented Feb 21, 2019

A procedural macro for generating static structures for use as the backend for config preferences, as well a mapping from string names to accessors.

Preferences can be accessed and updated via a map-like interface with String keys, and now also via a convenience macro: get_pref!(path.to.pref). Various serde-compatible field attributes are also supported, including renaming the string keys. This could be useful when changing the backend structure without breaking existing usages.

I have added the choice to use i64 as well as f64 for storing numbers. As it turns out, none of the existing preferences used non-integer values. Setting a floating point value from a command-line argument requires a decimal point in order to parse correctly.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #8539
  • There are tests for these changes OR
  • These changes do not require tests because ___

I have a few outstanding problems or questions:

  1. I am unable to get rid of this warning:
    warning: unnecessary path disambiguator
       --> components/config/prefs.rs:130:51
        |
    130 |         accessor_type = crate::pref_util::Accessor::<Prefs, crate::pref_util::PrefValue>,
        |                                                   ^^ try removing `::`
    
    See: https://stackoverflow.com/questions/54710797/how-to-disable-unnecessary-path-disambiguator-warning
  2. Several of the preferences in use were not represented in prefs.json. Some of these may be in error, but it is hard to tell. For example js.offthread_compilation.enabled vs js.ion.offthread_compilation.enabled could be different preferences or could be intended to have the same value.
  3. Previously, several pieces of code provided default values when accessing a preference that may not be present. For example:
    let DBL_CLICK_TIMEOUT = Duration::from_millis(
        PREFS
            .get("dom.document.dblclick_timeout")
            .as_u64()
            .unwrap_or(300),
    );
    Fallback values don't really make sense now and I've added these defaults to prefs.json. Does this sound right?
  4. I have kept PrefValue::Missing, though it doesn't seem very useful any more. An error might be more appropriate now for an incorrect preference key. I've kept this partly because webdriver_server uses it.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Feb 21, 2019

Heads up! This PR modifies the following files:

  • @jgraham: components/webdriver_server/lib.rs
  • @emilio: components/style/stylesheets/viewport_rule.rs, components/style/global_style_data.rs, components/style/properties/properties.mako.rs
  • @KiChjang: components/net/filemanager_thread.rs, components/net/lib.rs, components/script/dom/bindings/guard.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/servoparser/mod.rs and 16 more
  • @asajeffrey: components/script/dom/bindings/guard.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/servoparser/mod.rs, components/script/dom/mouseevent.rs, components/script/dom/htmlcanvaselement.rs and 15 more
  • @cbrewster: components/constellation/pipeline.rs, components/constellation/constellation.rs
  • @paulrouget: ports/servo/browser.rs, components/servo/lib.rs, ports/servo/non_android_main.rs, components/constellation/pipeline.rs, components/constellation/constellation.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Feb 21, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Feb 22, 2019

This looks like an improvement to me! @jdm, I believe you had some plans around config, is this PR in line with what you were thinking?

@peterjoel

This comment has been minimized.

Copy link
Contributor Author

peterjoel commented Feb 22, 2019

@asajeffrey There is some discussion with @jdm on the original issue #8539.

In the end, I was able to make both the static and key-based interface mutable.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 26, 2019

☔️ The latest upstream changes (presumably #22940) made this pull request unmergeable. Please resolve the merge conflicts.

@peterjoel peterjoel force-pushed the peterjoel:issue_8539_prefs_refactor branch 3 times, most recently from 89d7382 to 08e3c40 Feb 27, 2019

@jdm jdm removed the S-needs-rebase label Feb 27, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 27, 2019

☔️ The latest upstream changes (presumably #22939) made this pull request unmergeable. Please resolve the merge conflicts.

@peterjoel peterjoel force-pushed the peterjoel:issue_8539_prefs_refactor branch from 08e3c40 to 2a1e5d8 Feb 27, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 27, 2019

☔️ The latest upstream changes (presumably #22945) made this pull request unmergeable. Please resolve the merge conflicts.

@peterjoel peterjoel force-pushed the peterjoel:issue_8539_prefs_refactor branch from 2a1e5d8 to 30f2c23 Feb 28, 2019

@asajeffrey
Copy link
Member

asajeffrey left a comment

Mainly this looks really good! A few questions though...

Show resolved Hide resolved components/config/pref_util.rs

pub struct Accessor<P, V> {
pub getter: Box<Fn(&P) -> V + Sync>,
pub setter: Box<Fn(&mut P, V) + Sync>,

This comment has been minimized.

@asajeffrey

asajeffrey Feb 28, 2019

Member

Hmm, this is doing dynamic dispatch. Do we ever access a pref inside a hot loop?

This comment has been minimized.

@emilio

emilio Mar 1, 2019

Member

Yes, prefs are accessed all the time during CSS parsing.

This comment has been minimized.

@peterjoel

peterjoel Mar 1, 2019

Author Contributor

These accessors are only used for accessing preferences using their string keys. In most places the value is accessed directly from the structs (usually via the get_pref! macro.

This comment has been minimized.

@peterjoel

peterjoel Mar 1, 2019

Author Contributor

I will take a look at the CSS code to check if it is using the string names.

This comment has been minimized.

@peterjoel

peterjoel Mar 1, 2019

Author Contributor

I also realise that the accessors are used in every place that a preference is changed. In places where the preference name is statically known, I can actually change that to update the structs directly.

This comment has been minimized.

@peterjoel

peterjoel Mar 1, 2019

Author Contributor

@asajeffrey With regard to accessing preferences in CSS, I don't follow the Python code generation very well. Are there existing benchmarks for the CSS to make sure that nothing is degraded? I can't see a ./mach command for that.

This comment has been minimized.

@asajeffrey

asajeffrey Mar 1, 2019

Member

Don't know I'm afraid. @emilio?

This comment has been minimized.

@emilio

emilio Mar 6, 2019

Member

I don't think I know of anything else that isn't reloading a page full of stylesheets over and over. In any case, looks like we don't have that many pref-gated properties, so probably not something that should concern us much.

This comment has been minimized.

@peterjoel

peterjoel Mar 6, 2019

Author Contributor

@emilio can you point me to where some pref-gated CSS properties are implemented?

This comment has been minimized.

@emilio

emilio Mar 6, 2019

Member

Sure, just grep for servo_pref= in components/style/properties. Looks like we have a few of them gated with layout.columns.enabled, and then layout.writing-mode.enabled.

Show resolved Hide resolved components/config/pref_util.rs Outdated
Show resolved Hide resolved components/config/prefs.rs
Show resolved Hide resolved components/config/prefs.rs Outdated
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Feb 28, 2019

I just want to chime in and say that this was clearly more complex than I originally thought, and that I'm impressed by your solution!

@emilio
Copy link
Member

emilio left a comment

It is a bit unfortunate to have to pay the cost of locking and arc-cloning to access simple prefs. Ideally bool prefs should be just a relaxed atomic read. Though given this simplifies the API and makes it easy to change it under the hood, it may be worth to land it and improve that as a follow-up...

Show resolved Hide resolved components/config/prefs.rs
Show resolved Hide resolved components/config/prefs.rs Outdated
@peterjoel

This comment has been minimized.

Copy link
Contributor Author

peterjoel commented Mar 1, 2019

@emilio wrote:
It is a bit unfortunate to have to pay the cost of locking and arc-cloning to access simple prefs. Ideally bool prefs should be just a relaxed atomic read. Though given this simplifies the API and makes it easy to change it under the hood, it may be worth to land it and improve that as a follow-up...

The existing implementation stores the preferences in a Arc<RwLock<HashMap<String, Pref>>>, but each Pref also wraps the value in another Arc. Hopefully, this new implementation is at least no worse than that.

The choice was between locking the entire structure or locking individual preferences, and I went with the simplest one. Since most code can use the macros, as you say, it can be changed under the hood.

Show resolved Hide resolved components/config/opts.rs Outdated
@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Mar 1, 2019

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15617.

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Mar 1, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15617.

@peterjoel peterjoel force-pushed the peterjoel:issue_8539_prefs_refactor branch from 101eb80 to 675b494 Mar 1, 2019

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Mar 1, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15617.

1 similar comment
@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Mar 1, 2019

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15617.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 19, 2019

@bors-servo r=asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 19, 2019

📌 Commit 73e47e9 has been approved by asajeffrey

bors-servo added a commit that referenced this pull request Mar 19, 2019

Auto merge of #22923 - peterjoel:issue_8539_prefs_refactor, r=asajeffrey
#8539 Config preferences backend restructure

<!-- Please describe your changes on the following line: -->

A procedural macro for generating static structures for use as the backend for config preferences, as well a mapping from string names to accessors.

Preferences can be accessed and updated via a map-like interface with `String` keys, and now also via a convenience macro: `get_pref!(path.to.pref)`. Various `serde`-compatible field attributes are also supported, including renaming the string keys. This could be useful when changing the backend structure without breaking existing usages.

I have added the choice to use `i64` as well as `f64` for storing numbers. As it turns out, none of the existing preferences used non-integer values. Setting a floating point value from a command-line argument requires a decimal point in order to parse correctly.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8539

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

-----

I have a few outstanding problems or questions:

1. I am unable to get rid of this warning:
    ```
    warning: unnecessary path disambiguator
       --> components/config/prefs.rs:130:51
        |
    130 |         accessor_type = crate::pref_util::Accessor::<Prefs, crate::pref_util::PrefValue>,
        |                                                   ^^ try removing `::`
    ```
    See: https://stackoverflow.com/questions/54710797/how-to-disable-unnecessary-path-disambiguator-warning
2. Several of the preferences in use were not represented in `prefs.json`. Some of these may be in error, but it is hard to tell. For example `js.offthread_compilation.enabled` vs `js.ion.offthread_compilation.enabled` could be different preferences or could be intended to have the same value.
3. Previously, several pieces of code provided default values when accessing a preference that may not be present. For example:
    ```Rust
    let DBL_CLICK_TIMEOUT = Duration::from_millis(
        PREFS
            .get("dom.document.dblclick_timeout")
            .as_u64()
            .unwrap_or(300),
    );
    ```
    Fallback values don't really make sense now and I've added these defaults to `prefs.json`. Does this sound right?
4. I have kept `PrefValue::Missing`, though it doesn't seem very useful any more. An error might be more appropriate now for an incorrect preference key. I've kept this partly because [`webdriver_server` uses it](https://github.com/servo/servo/blob/master/components/webdriver_server/lib.rs#L224).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22923)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 19, 2019

⌛️ Testing commit 73e47e9 with merge db1685a...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 19, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 19, 2019

dom.testbinding.preference_value.quote_string_test

@peterjoel peterjoel force-pushed the peterjoel:issue_8539_prefs_refactor branch from 73e47e9 to 8bfd4dc Mar 20, 2019

@peterjoel

This comment has been minimized.

Copy link
Contributor Author

peterjoel commented Mar 20, 2019

@jdm I also found browser.display.background-color and browser.display.foreground-color, which I've also added now. Hopefully that's the last.🤞

@peterjoel

This comment has been minimized.

Copy link
Contributor Author

peterjoel commented Mar 20, 2019

@jdm I've come across some mention of prefs in some Python tests, e.g: https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/tests/test_wpttest.py#L14 :

I'm not exactly sure what these are doing though, or how to make sure that I've run them properly. Are these trying to set and reset a pref called "a"?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 20, 2019

Those are python-only unit tests for wptrunner and don't run against Servo.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 20, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 20, 2019

📌 Commit 8bfd4dc has been approved by jdm

@highfive highfive assigned jdm and unassigned asajeffrey Mar 20, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 20, 2019

⌛️ Testing commit 8bfd4dc with merge 5ec7254...

bors-servo added a commit that referenced this pull request Mar 20, 2019

Auto merge of #22923 - peterjoel:issue_8539_prefs_refactor, r=jdm
#8539 Config preferences backend restructure

<!-- Please describe your changes on the following line: -->

A procedural macro for generating static structures for use as the backend for config preferences, as well a mapping from string names to accessors.

Preferences can be accessed and updated via a map-like interface with `String` keys, and now also via a convenience macro: `get_pref!(path.to.pref)`. Various `serde`-compatible field attributes are also supported, including renaming the string keys. This could be useful when changing the backend structure without breaking existing usages.

I have added the choice to use `i64` as well as `f64` for storing numbers. As it turns out, none of the existing preferences used non-integer values. Setting a floating point value from a command-line argument requires a decimal point in order to parse correctly.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #8539

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

-----

I have a few outstanding problems or questions:

1. I am unable to get rid of this warning:
    ```
    warning: unnecessary path disambiguator
       --> components/config/prefs.rs:130:51
        |
    130 |         accessor_type = crate::pref_util::Accessor::<Prefs, crate::pref_util::PrefValue>,
        |                                                   ^^ try removing `::`
    ```
    See: https://stackoverflow.com/questions/54710797/how-to-disable-unnecessary-path-disambiguator-warning
2. Several of the preferences in use were not represented in `prefs.json`. Some of these may be in error, but it is hard to tell. For example `js.offthread_compilation.enabled` vs `js.ion.offthread_compilation.enabled` could be different preferences or could be intended to have the same value.
3. Previously, several pieces of code provided default values when accessing a preference that may not be present. For example:
    ```Rust
    let DBL_CLICK_TIMEOUT = Duration::from_millis(
        PREFS
            .get("dom.document.dblclick_timeout")
            .as_u64()
            .unwrap_or(300),
    );
    ```
    Fallback values don't really make sense now and I've added these defaults to `prefs.json`. Does this sound right?
4. I have kept `PrefValue::Missing`, though it doesn't seem very useful any more. An error might be more appropriate now for an incorrect preference key. I've kept this partly because [`webdriver_server` uses it](https://github.com/servo/servo/blob/master/components/webdriver_server/lib.rs#L224).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22923)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 20, 2019

@bors-servo bors-servo merged commit 8bfd4dc into servo:master Mar 20, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@jdm

This comment has been minimized.

Copy link
Member

jdm commented Mar 20, 2019

Woohoo! Great work, @peterjoel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.