Improve ergonomics to use Gecko prefs from rust
Categories
(Core :: Preferences: Backend, task)
Tracking
()
People
(Reporter: padenot, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 2 obsolete files)
Emilio tells me that something hacky exists, but only for StaticPrefs, used in Servo, but it would be nice to have something a bit better and safe to use Gecko prefs from rust code.
|
Reporter | |
Comment 1•6 years ago
|
||
It's a bit annoying that this is using a class with mostly static members, and I think only static members when it comes to the external interface that Gecko developers doing rust want to use.
Also this uses quite a lot of overloading and the names are going to be mangled anyways, so having something (bindgen) do it automatically is probably a lost cause.
We should maybe just do a crate with simple functions that expose the simple Get* and Set* API (not the *Complex API), that would cover most usage of this from rust code. Callbacks would be good to have but I'm afraid that it's going to be a bit hard to bind, it's a bit template heavy.
Emilio, thoughts? I can have a go at it (I need it for something), but feel free to steal.
|
Assignee | |
Comment 2•6 years ago
|
||
Paul, can you give a specific example of something that you want to do that you currently cannot? libpref is (unfortunately) sufficient complex that "use Gecko prefs from Rust" could mean a variety of things.
|
|
Reporter | |
Comment 3•6 years ago
|
||
Get the value of a pref from a rust crate, at run time, in a content process however quite early (just before the sandbox lockdown).
|
||
Comment 5•6 years ago
|
||
The kind of thing I was thinking of was something like:
- Moving StaticPrefList.py to Python / toml / json / something.
- Use that to generate both
StaticPrefList.h, and apref!macro from rust much like theatom!macro works today, potentially just calling into C++ code.
The way the macro could work could just be having an FFI function like:
extern "C" Type StaticPrefs_foo_bar() {
return StaticPrefs::foo_bar();
}
And in rust an auto-generated macro like:
macro_rules! pref {
("foo.bar") => {{ unsafe { StaticPrefs_foo_bar() }},
// ...
}
So the rust usage would look like if something && pref!("foo.bar") {...
If StaticPrefs allow for Paul's use-case it'd be nice, since it'd clean up pref usage from the style system code as well.
|
|
Assignee | |
Comment 6•6 years ago
|
||
So the rust usage would look like
if something && pref!("foo.bar").
As opposed to the current if something && sVarCache_foo_bar, right? I can see it's nicer to write, and that's worth having, but I don't think it provides new functionality. Or am I missing something? I don't understand the complaint about static members in comment 1.
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 7•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6)
As opposed to the current
if something && sVarCache_foo_bar, right? I can see it's nicer to write, and that's worth having, but I don't think it provides new functionality. Or am I missing something? I don't understand the complaint about static members in comment 1.
You can only use the sVarCache stuff from style system code, which is the one that runs bindgen atm. So most rust code in m-c doesn't have that capability.
|
|
Reporter | |
Comment 8•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4)
Which pref or prefs?
security.sandbox.content.level, for now.
My complaint about static methods was that it means we'll have to wrap all of them in functions with a C mangling, instead of wrapping them directly.
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 9•6 years ago
|
||
The kind of thing I was thinking of was something like:
- Moving StaticPrefList.py to Python / toml / json / something.
- Use that to generate both
StaticPrefList.h,
This is now done. modules/libpref/init/StaticPrefList.yaml is the definitions file, and modules/libpref/init/generate_static_pref_list.py is the generator script.
and a
pref!macro from rust much like theatom!macro works today, potentially just calling into C++ code.The way the macro could work could just be having an FFI function like:
extern "C" Type StaticPrefs_foo_bar() { return StaticPrefs::foo_bar(); }
All those functions would be in one generated .cpp file somewhere, right? Would there need to be a .h file with the declarations as well?
And in rust an auto-generated macro like:
macro_rules! pref { ("foo.bar") => {{ unsafe { StaticPrefs_foo_bar() }}, // ... }
Likewise, would all these definitions be in a single .rs file? Would that .rs file be in its own crate?
We're close to having this, but the structure of the generated code is still a bit unclear to me, hence my questions.
|
|
Assignee | |
Comment 10•6 years ago
|
||
My complaint about static methods was that it means we'll have to wrap all of them in functions with a C mangling, instead of wrapping them directly.
Sorry, I still don't understand this fully. Can you explain more what you mean by "wrapping them directly?"
Here's how I'm imagining all this will work, based on what Emilio wrote above.
- If you want to access a pref from Rust, you'll have to put it into
StaticPrefList.yaml. - That will cause a C FFI function to be generated, along with a Rust macro that calls the function.
- You can then call the macro from your Rust code.
|
|
||
Comment 11•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #9)
The kind of thing I was thinking of was something like:
- Moving StaticPrefList.py to Python / toml / json / something.
- Use that to generate both
StaticPrefList.h,This is now done.
modules/libpref/init/StaticPrefList.yamlis the definitions file, andmodules/libpref/init/generate_static_pref_list.pyis the generator script.
That's great, thanks! \o/
and a
pref!macro from rust much like theatom!macro works today, potentially just calling into C++ code.The way the macro could work could just be having an FFI function like:
extern "C" Type StaticPrefs_foo_bar() { return StaticPrefs::foo_bar(); }All those functions would be in one generated
.cppfile somewhere, right? Would there need to be a.hfile with the declarations as well?
Not necessarily. You can have it, but I don't think anything needs to use the .h file, so as long as the rust code has all the declarations, and the cpp code all the definitions it should be fine.
And in rust an auto-generated macro like:
macro_rules! pref { ("foo.bar") => {{ unsafe { StaticPrefs_foo_bar() }}, // ... }Likewise, would all these definitions be in a single
.rsfile? Would that.rsfile be in its own crate?
Probably? And probably too, so that the rest of Gecko and not only style can read them. That has the issue of touching StaticPrefList.yaml causing all the rust dependencies of this crate to get rebuilt. You could do something like what you've done in bug 1563139 but with crates, but it seems a bit more overkill, I don't know.
|
|
Assignee | |
Comment 12•6 years ago
|
||
|
|
Assignee | |
Comment 13•6 years ago
|
||
I have a draft patch working. The generated C++ code looks like this:
extern "C" int32_t StaticPrefs_dom_min_timeout_value() {
return StaticPrefs::dom_min_timeout_value();
}
extern "C" bool StaticPrefs_layout_css_font_display_enabled() {
return StaticPrefs::layout_css_font_display_enabled();
}
The generated Rust code looks like this:
extern "C" {
pub fn StaticPrefs_dom_min_timeout_value() -> i32;
pub fn StaticPrefs_layout_css_font_display_enabled() -> bool;
}
#[macro_export]
macro_rules! static_pref {
("dom.min_timeout_value") => {{
unsafe { StaticPrefs_dom_min_timeout_value() }
}};
("layout.css.font-display.enabled") => {{
unsafe { StaticPrefs_layout_css_font_display_enabled() }
}};
}
Rust code that uses the prefs looks like this:
use static_prefs::*;
let pref = static_pref!("layout.css.font-display.enabled");
All the generated C++ code is in a single file. That file will get recompiled any time StaticPrefList.yaml changes, but it's only one .cpp file so that's not bad. I guess it could be split up into pieces to slightly reduce the amount of recompilation that occurs.
All the generated Rust code will also go into a single file. Splitting it into pieces seems much harder, and hopefully incremental compilation will handle things nicely to minimize recompilation costs there anyway...
Emilio: Does this look reasonable to you? Any improvements you can think of?
|
|
||
Comment 14•6 years ago
|
||
That all looks pretty great to me. I agree that splitting the rust code is probably not worth it. Same for the cpp file.
Are the StaticPrefs::foo_bar_enabled() calls inlined out of curiosity? If not, it may be worth moving the generated cpp file to the same translation unit as where they're defined... Though maybe we're doing enough LTO and co that it doesn't matter at all in practice.
The use static_prefs::* is a bit unfortunate, I think we can avoid it easily by using $crate in the generated macro. That'd only require to import the macro itself.
I wouldn't bother with the static_ prefix in the macro name, I think we should encourage the usage of it and make it shorter. Maybe just pref!? that'd also match the name Servo uses for their macros, which would save a bunch of conditional compilation in some cases. But if you feel strongly about that then this is fine of course.
|
|
Assignee | |
Comment 15•6 years ago
|
||
Are the
StaticPrefs::foo_bar_enabled()calls inlined out of curiosity? If not, it may be worth moving the generated cpp file to the same translation unit as where they're defined... Though maybe we're doing enough LTO and co that it doesn't matter at all in practice.
The C++ StaticPrefs::foo_bar_enabled() functions are defined as inline in StaticPrefListBegin.h, and the generated files containing the C StaticPrefs_foo_bar_enabled() function include that header file. So the C++ getter should be inlined into the C function. (I think that's what you're asking.)
The
use static_prefs::*is a bit unfortunate, I think we can avoid it easily by using$cratein the generated macro. That'd only require to import the macro itself.
Good suggestion, it does work.
I wouldn't bother with the
static_prefix in the macro name, I think we should encourage the usage of it and make it shorter. Maybe justpref!? that'd also match the name Servo uses for their macros, which would save a bunch of conditional compilation in some cases. But if you feel strongly about that then this is fine of course.
I have ended up using static_prefs::pref!("foo.bar.baz") in most places. What do you think about that? Having said that, it seems like all this code is marked with #[cfg(feature = "gecko")], so I don't understand why the Servo macros matter.
|
|
Assignee | |
Comment 16•6 years ago
|
||
generate_static_pref_list currently has two functions:
- generate_header(): this checks the YAML and generates most of the code.
- emit_header(): this does a bit of additional code generation and writes the
code to file.
This patch gives it a cleaner structure:
- check_pref_list(): this checks the YAML.
- generate_code(): this generates all the code. (It calls check_pref_list()
first.) - emit_code(): this just writes the code to file.
The patch also improves the testing in test_generate_static_pref_list.py,
so that it checks the full contents of all generated files.
All these improvements will help with the next patch, which adds two additional
generated files.
|
||
Updated•6 years ago
|
|
|
Assignee | |
Comment 17•6 years ago
|
||
This patch introduces a new Rust crate called static_prefs.
It also changes generate_static_pref_list.py to generate two new files.
-
StaticPrefsCGetters.cpp: contains C getters, which are just wrappers around
the C++ getters. This is included into Preferences.cpp. -
static_prefs.rs: contains declarations for the C getters, plus the
pref!
macro which provides nice syntax for calling the C getters. This is included
into static_prefs/src/lib.rs.
The new code is only generated for prefs marked with the new rust field in
the YAML. It's opt-in because there's no point generating additional code for
900+ static prefs when only about 20 are currently used from Rust.
This patch only marks a single pref (browser.display.document_color_use) with
rust: true. That pref isn't accessed from Rust code in this patch, but it's
necessary because the generated Rust code is invalid if there are zero
Rust-accessed prefs. (The next patch will access that pref and others from Rust
code.
Depends on D40790
|
|
Assignee | |
Comment 18•6 years ago
|
||
It's much nicer.
One nice thing about this is that the new code is subject to the existing
threadedness checking, which identified that several of these should be atomic
because they're accessed off the main thread.
Depends on D40791
|
|
Assignee | |
Comment 19•6 years ago
|
||
Bindgen is no longer necessary now that Rust bindings are generated by
generate_static_pref_list.py.
Depends on D40792
|
|
Assignee | |
Comment 20•6 years ago
|
||
Another step in the renaming of VarCache variables as mirror variables,
matching the 'mirror' field used in StaticPrefList.yaml.
Depends on D40793
|
||
Comment 21•6 years ago
|
||
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 22•6 years ago
|
||
|
||
Comment 23•6 years ago
•
|
||
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
|
||
Updated•6 years ago
|
|
|
||
Comment 24•6 years ago
|
||
| bugherder | ||
|
|
||
Comment 25•6 years ago
|
||
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 26•6 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•6 years ago
|
|
|
||
Comment 27•6 years ago
|
||
|
|
||
Comment 28•6 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/aaa04efb13f9
https://hg.mozilla.org/mozilla-central/rev/40f5cf9e18d1
|
||
Updated•6 years ago
|
Description
•