Closed
Bug 1436655
Opened 6 years ago
Closed 6 years ago
Introduce a mechanism for prefs to be defined entirely in the binary
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(4 files)
I think this will improve things greatly.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 3•6 years ago
|
||
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. I'm casting a wide feedback net here because it's a big change! Thanks.
Attachment #8949290 -
Flags: review?(nfroyd)
Attachment #8949290 -
Flags: review?(mh+mozilla)
Attachment #8949290 -
Flags: feedback?(rnewman)
Attachment #8949290 -
Flags: feedback?(nfroyd)
Attachment #8949290 -
Flags: feedback?(mh+mozilla)
Attachment #8949290 -
Flags: feedback?(felipc)
Attachment #8949290 -
Flags: feedback?(erahm)
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. https://reviewboard.mozilla.org/r/218672/#review224610 This seems like a step in the right direction! ::: modules/libpref/init/All.h:10 (Diff revision 1) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +// This file defines VarCache prefs. > +// > +// Prefs defined in this file should *not* be listed in a prefs data file such > +// as all.js. What happens if they are? Can we check for these named varcache prefs and err or warn appropriately if they're present when reading a prefs file? ::: modules/libpref/init/All.h:22 (Diff revision 1) > +// functions. > +// > +// - A direct global variable access is faster than a hash table lookup. > +// > +// - A global variable can be accessed off the main thread. If a pref *is* > +// accessed off the main thread, it should use an atomic type. (But note that Can we add a debugging thread check to the generated accessor (excepting `Atomic<float>`, of course)? ::: modules/libpref/init/All.h:24 (Diff revision 1) > +// - A direct global variable access is faster than a hash table lookup. > +// > +// - A global variable can be accessed off the main thread. If a pref *is* > +// accessed off the main thread, it should use an atomic type. (But note that > +// many VarCaches that should be atomic are not, in particular because > +// Atomic<float> is not available, alas.) Does that mean we currently do cross-thread reads that can return garbage values? Can you file a bug if necessary, and point to it from here? ::: modules/libpref/init/All.h:35 (Diff revision 1) > +// <c++-type>, <default-value>) > +// > +// - The macro name is PRF because then subsequent lines can be indented by 4 > +// spaces, which is nice. > +// > +// - <pref-name-string> is the name of the pref, as it appears in about:config, Are there any C++ users of the name?
Attachment #8949290 -
Flags: review+
Comment 5•6 years ago
|
||
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. (ReviewBoard)
Attachment #8949290 -
Flags: review+
Attachment #8949290 -
Flags: feedback?(rnewman)
Attachment #8949290 -
Flags: feedback+
![]() |
Assignee | |
Comment 6•6 years ago
|
||
Thank you for the comments. > > +// Prefs defined in this file should *not* be listed in a prefs data file such > > +// as all.js. > > What happens if they are? Can we check for these named varcache prefs and > err or warn appropriately if they're present when reading a prefs file? Currently I think the value from the file will be installed normally, overwriting the value obtained from the VarCache installation. It would be possible to add a check to detect this case. > > +// - A global variable can be accessed off the main thread. If a pref *is* > > +// accessed off the main thread, it should use an atomic type. (But note that > > Can we add a debugging thread check to the generated accessor I think so. The exception is for VarCache variables accessed via Rust code, because they do raw variable accesses instead of using the C++ getter. > (excepting `Atomic<float>`, of course)? (BTW, in the same way that floats are actually stored as strings in the hash table and then converted at use points, I'm wondering if float VarCache values could actually be stored as integers, in order to get atomicity.) > > +// - A global variable can be accessed off the main thread. If a pref *is* > > +// accessed off the main thread, it should use an atomic type. (But note that > > +// many VarCaches that should be atomic are not, in particular because > > +// Atomic<float> is not available, alas.) > > Does that mean we currently do cross-thread reads that can return garbage > values? Can you file a bug if necessary, and point to it from here? Currently: the main thread writes the global variables, and other threads can read them. If other threads do read them and the global variable is non-atomic, then the consequences are... unpredictable. Possibly garbage. I filed bug 1436916 for this. > > +// - <pref-name-string> is the name of the pref, as it appears in about:config, > > Are there any C++ users of the name? The next line in the comment says "and as is used in most libpref API functions." I will reword the comment to indicate this covers both C++ and JS API functions.
![]() |
||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. https://reviewboard.mozilla.org/r/218672/#review225358 ::: modules/libpref/StaticPrefs.h:53 (Diff revision 1) > +private: \ > + static cpp_type sVarCache_##id; \ > + \ > +public: \ > + static StripAtomic<cpp_type> id() { return sVarCache_##id; } > +#include "mozilla/All.h" Nit: can we call this something else other than `All.h`? ::: modules/libpref/init/All.h:67 (Diff revision 1) > +PRF("layout.accessiblecaret.width", > + layout__accessiblecaret__width, Are there any plans to auto-generate the C++-side names? I can imagine that people will complain about the naming algorithm and start using some more palatable in some cases, which makes things less understandable for everybody.
Attachment #8949290 -
Flags: review?(nfroyd)
![]() |
||
Comment 8•6 years ago
|
||
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. This seems reasonable.
Attachment #8949290 -
Flags: review?(nfroyd)
Attachment #8949290 -
Flags: feedback?(nfroyd)
Attachment #8949290 -
Flags: feedback+
![]() |
Assignee | |
Comment 9•6 years ago
|
||
> > +#include "mozilla/All.h" > > Nit: can we call this something else other than `All.h`? Sure. Any suggestions? Prefs.h? > > +PRF("layout.accessiblecaret.width", > > + layout__accessiblecaret__width, > > Are there any plans to auto-generate the C++-side names? AFAICT that would require switching to using a script to do the code generation instead of the C preprocessor, which I was hoping to avoid because it's a whole extra step. Unless you can think of another way?
Comment 10•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #7) > Are there any plans to auto-generate the C++-side names? I also thought this would be nice, but didn't mention it in my comment for two reasons: - I didn't think it was possible without extra work, and - The explicit way keeps the names in the tree, right in the definition, which will help searchers. To address the second, we'd need a macro at point of use, too. E.g., Rust: let foo = pref!(layout.foo.bar); which would expand to let foo = StaticPrefs::layout__foo__bar();
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8949289 [details] Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit. https://reviewboard.mozilla.org/r/218670/#review225508
Attachment #8949289 -
Flags: review?(mh+mozilla) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. https://reviewboard.mozilla.org/r/218672/#review226312 ::: commit-message-de2a3:23 (Diff revision 1) > + > +This patch introduces a new way of doing things. There is a new file, > +modules/libpref/init/All.h, which defines prefs like this: > + > +> PRF("layout.accessiblecaret.width", > +> layout__accessiblecaret__width, Please do not use double underscores. They are always reserved for compilers.
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. https://reviewboard.mozilla.org/r/218672/#review226298 Some notes inline, mainly I think we should go ahead and add a script for generating the code rather than just using macros. f=me ::: caps/nsScriptSecurityManager.cpp:677 (Diff revision 1) > // A null principal can target its own URI. > if (sourceURI == aTargetURI) { > return NS_OK; > } > } > - else if (sViewSourceReachableFromInner && > + else if (StaticPrefs::security__view_source__reachable_from_inner_protocol() && The ergonomics of this aren't great, it's more verbose than before (the need to specify the full path of the pref is unfortunate) and the user can't define the var name as they've been used to. I'm not saying you shouldn't do this, but maybe ping dev-platform for alternative ideas? Or maybe this is as good as it gets. I *do* think it's nice that we're preventing direct access to var so you can't accidentally modify it and of course avoiding a lot of boilerplate is nice as well. ::: modules/libpref/Preferences.cpp:4708 (Diff revision 1) > + > + Preferences::AddUintVarCache(aCache, aName, aDefaultValue, true); > +} > + > +static void > +RegisterStaticPref(const char* aName, float* aCache, const char* aDefaultValue) Seems like `aDefaultValue` shouldn't need to be a string. Either we could pass in just the float value and stringify via AppendFloat or we could pass in the float value *and* a macro-fu stringified value. ::: modules/libpref/StaticPrefs.h:49 (Diff revision 1) > +// njn: could assert in the getter that it's been initialized... > +// [but this would require moving it back into the .cpp file to access the > +// isInitialized data...] > +#define PRF(str, id, cpp_type, value) \ > +private: \ > + static cpp_type sVarCache_##id; \ One argument for generating this with a script is that we could attempt to order the prefs so that the struct packs well. We should also be able to initialize the values right? ::: modules/libpref/init/All.h:44 (Diff revision 1) > +// the StaticPrefs class. For consistency, the identifier for every pref > +// should be created like so: > +// - start with <pref-name-string>; > +// - convert any '.' chars to '__'; > +// - convert any '-' chars to '_'. > +// For example, "foo.bar_baz" becomes |foo__bar_baz|. Ugly, but clear! The ergonomics of this are rough, but I don't have a better solution. I agree with previous comments that this adding a transform step rather than just macros would help with folks not adhearing to the rules.
Updated•6 years ago
|
Attachment #8949290 -
Flags: feedback?(erahm) → feedback+
![]() |
Assignee | |
Comment 14•6 years ago
|
||
> > +static void > > +RegisterStaticPref(const char* aName, float* aCache, const char* aDefaultValue) > > Seems like `aDefaultValue` shouldn't need to be a string. True. I'm planning to extend this scheme to non-VarCache prefs as well, and there's a danger case where a person may specify an integer literal for a pref that's meant to be a float and the system could install it as an int pref instead of a float-as-string pref, and then lookups won't work properly. But I think I can defend against that case if I'm careful. > We should also be able to initialize the values right? An argument against that is that if they're all initially zero then they should end up in the .bss section, which doesn't take up any disk space.
![]() |
Assignee | |
Comment 15•6 years ago
|
||
> Please do not use double underscores. They are always reserved for compilers.
Good point! Thank you. I'll switch to using single underscores to replace both '.' and '-'. It's a lossy conversion (i.e. you can't get the original string back from the identifer) but I think it's good enough.
![]() |
Assignee | |
Comment 16•6 years ago
|
||
> I *do* think it's nice that we're preventing direct access to var so you > can't accidentally modify it Yes -- see bug 1438433 where I am fixing exactly such a modification :)
Comment 17•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #14) > An argument against that is that if they're all initially zero then they > should end up in the .bss section, which doesn't take up any disk space. But in that case the values are still inline in the code. They still take space. Sometimes even more. #include <stdlib.h> size_t foo; char* bar; __attribute__((constructor)) void init() { foo = 1; bar = "bar"; } 00000000000004e0 <init>: 4e0: 48 8d 05 fe 00 00 00 lea 0xfe(%rip),%rax # 5e5 <_fini+0x9> 4e7: 48 c7 05 3e 0b 20 00 movq $0x1,0x200b3e(%rip) # 201030 <foo> 4ee: 01 00 00 00 4f2: 48 89 05 2f 0b 20 00 mov %rax,0x200b2f(%rip) # 201028 <bar> 4f9: c3 retq 4fa: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) First instruction (7 bytes) calculates the pointer to "bar", and the third (7) stores it in bar. That's 14 bytes for a string literal. Second instruction (11 bytes) puts 1 in foo. That's 25 bytes of code, 4 bytes for the literal string, and 16 bytes of .bss. Compare with: #include <stdlib.h> size_t foo = 1; char* bar = "bar"; No code, still 4 bytes for the literal string, and 16 bytes of .data. ... but there's also one relocation to fill the pointer that didn't exist before, and on Linux, those are not cheap in space (24 bytes on 64-bits iirc ; except with elfhack, if they're next to other relocated data). So filling strings with a function can win in space, but not integers.
Comment 18•6 years ago
|
||
well, except if you do char bar[] = "bar", because then you don't get a relocation at all.
Comment 19•6 years ago
|
||
(except you can't reassign to that, obviously)
Updated•6 years ago
|
Attachment #8949290 -
Flags: feedback?(mh+mozilla) → feedback+
![]() |
Assignee | |
Updated•6 years ago
|
Summary: Introduce a mechanism for VarCache prefs to be defined entirely in the binary → Introduce a mechanism for prefs to be defined entirely in the binary
Comment 20•6 years ago
|
||
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. Looks great to me too! The one thing I disagree is about defining the default value here, instead of the existing prefs files. You say that it was to consolidate these in one place instead of two, but I believe it's the other way around: now there's one extra place to look for prefs when trying to find where a specific pref is defined, and what is its default value I believe this feature should only be about mapping (pref name) -> (variable), without including its value. Which brings the question: what happens _if_ the pref is also defined in all.js or firefox.js? - If all.js and firefox.js is able to correctly override the value, then in reality you don't really need to have it defined here too - If they are not able to override it, then this will break the standard behavior and hierarchy of pref definitions, and it's guaranteed that this will lead to bugs. E.g. when someone does a last-minute pref flip in firefox.js (to disable a feature that is not ready to ship), but that definition doesn't take effect because it's defined here. (just my 2 cents)
Attachment #8949290 -
Flags: feedback?(felipc) → feedback+
![]() |
Assignee | |
Comment 21•6 years ago
|
||
> - If all.js and firefox.js is able to correctly override the value, then in > reality you don't really need to have it defined here too From comment 6: > > > +// Prefs defined in this file should *not* be listed in a prefs data file such > > > +// as all.js. > > > > What happens if they are? Can we check for these named varcache prefs and > > err or warn appropriately if they're present when reading a prefs file? > > Currently I think the value from the file will be installed normally, > overwriting the value obtained from the VarCache installation. It would be > possible to add a check to detect this case. One overarching goal is to define each pref in a single place. For C++ prefs (esp. VarCache ones) that will be in the new C++ file (All.h, or whatever name we end up using). It may be useful to allow people to override those default values via all.js -- it can saves a lot of recompiling when experimenting with different values. But that should be an exceptional case, perhaps one that triggers a warning that would be detected on automation.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 26•6 years ago
|
||
Some notable changes from the old version, largely in response to comments: - init/All.h has been renamed init/StaticPrefList.h. - No use of double underscores in static pref identifiers. - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file such as all.js. - Float pref default values are now specified as floats in StaticPrefList.h, rather than strings. - All VarCache variables for static prefs are now initialized in the binary, rather than at registration time. - It still uses the preprocessor rather than code generation. It's simpler, and nsCSSPropList.h is precedent for macros of this style that require both a string form and an identifier form :)
Comment 27•6 years ago
|
||
There are several concerns about this approach: 1. we would not be able to access prefs from Rust code like what we currently do in stylo (via bindings of static member fields generated by bindgen), which may lead to regressed performance and code scalability 2. any change to that list would trigger a rebuild on a much wider range than current
![]() |
Assignee | |
Comment 28•6 years ago
|
||
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #27) > There are several concerns about this approach: > 1. we would not be able to access prefs from Rust code like what we > currently do in stylo (via bindings of static member fields generated by > bindgen), which may lead to regressed performance and code scalability Pref access from Rust code will be very similar to what we already have. See bug 1448225. > 2. any change to that list would trigger a rebuild on a much wider range > than current Yes. That's the main downside of the patch, as described in the commit message. I think it's a worthwhile trade-off given the large number of benefits.
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8961633 [details] Bug 1436655 - Tweak the comment at the top of init/all.js. https://reviewboard.mozilla.org/r/230502/#review236850 ::: modules/libpref/init/all.js:8 (Diff revision 1) > - * for example xpfe/bootstrap/browser-prefs.js > + * for example browser/defaults/preferences/firefox.js. > * > - * Platform-specific #ifdefs at the end of this file override the generic > - * entries at the top. > + * For the syntax used by this file, consult the comments at the top of > + * modules/libpref/parser/src/lib.rs. nit: one of those paths is a path in the build, and the other in the source tree.
Attachment #8961633 -
Flags: review?(mh+mozilla) → review+
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8961634 [details] Bug 1436655 - Tweak assertions in Add*VarCache() functions. https://reviewboard.mozilla.org/r/230504/#review236852
Attachment #8961634 -
Flags: review?(mh+mozilla) → review+
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8949290 [details] Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. https://reviewboard.mozilla.org/r/218672/#review236860
Attachment #8949290 -
Flags: review?(mh+mozilla) → review+
![]() |
Assignee | |
Comment 32•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c34cc8156a71b1feab6cf4351b80ac60cd12cc78 Bug 1436655 - Rename pref_SetPref()'s aFromFile argument as aFromInit. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/8509ebf8fbb3af7e580c85aa73d5bb4a9b78c61a Bug 1436655 - Tweak the comment at the top of init/all.js. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/08af6778dd6a669e896d61ef7599a75d7024d5fd Bug 1436655 - Tweak assertions in Add*VarCache() functions. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/16ac5bd4e50c50a62316828b586c5e7a60b28e99 Bug 1436655 - Introduce a mechanism for VarCache prefs to be defined entirely in the binary. r=glandium
Comment 33•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #26) > - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file > such > as all.js. Can we make a test that will fail when this situation happens? Two nits I noticed when reading the documentation comments: > +// Prefs defined in this file should *not* be listed in a prefs data file such > +// as all.js. If they are, CheckForExistence() will issue an NS_ERROR. s/CheckForExistence/CheckForDoubleDefinition/ > +// ... Note that float prefs are > +// stored internally as floats. as strings
![]() |
Assignee | |
Comment 34•6 years ago
|
||
> > - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file > > such > > as all.js. > > Can we make a test that will fail when this situation happens? There's no need -- if it happens the presence of the NS_ERROR will cause various tests to fail. (I did this by accident with one try push while developing the patch!) > Two nits I noticed when reading the documentation comments: Thanks, I will fix.
![]() |
Assignee | |
Comment 35•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dabcac07c6835679aaeb585b3d991f5a805a3ffb Bug 1436655 - Fix up two minor mistakes in comments. r=me
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c34cc8156a71 https://hg.mozilla.org/mozilla-central/rev/8509ebf8fbb3 https://hg.mozilla.org/mozilla-central/rev/08af6778dd6a https://hg.mozilla.org/mozilla-central/rev/16ac5bd4e50c https://hg.mozilla.org/mozilla-central/rev/dabcac07c683
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Assignee: nobody → n.nethercote
![]() |
Assignee | |
Comment 37•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #34) > > > - It now NS_ERRORs if you have a pref in StaticPrefsList.h and a data file > > > such > > > as all.js. > > > > Can we make a test that will fail when this situation happens? > > There's no need -- if it happens the presence of the NS_ERROR will cause > various tests to fail. (I did this by accident with one try push while > developing the patch!) Unfortunately, this caused bustage for Thunderbird. The prefs network.auth.subresource-img-cross-origin-http-auth-allow and network.auth.non-web-content-triggered-resources-http-auth-allow are now defined in StaticPrefList.h, but Thunderbird overrides their values in all-thunderbird.js. (See bug 1449143.) The checking fundamentally isn't compatible with Thunderbird operating in this way. I think I'm just going to have to disable the checking :(
![]() |
||
Comment 38•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #37) > Unfortunately, this caused bustage for Thunderbird. The prefs > network.auth.subresource-img-cross-origin-http-auth-allow and > network.auth.non-web-content-triggered-resources-http-auth-allow are now > defined in StaticPrefList.h, but Thunderbird overrides their values in > all-thunderbird.js. (See bug 1449143.) > > The checking fundamentally isn't compatible with Thunderbird operating in > this way. I think I'm just going to have to disable the checking :( Could we disable the check only if we're building Thunderbird? Set a define in moz.build based on the value of MOZ_BUILD_APP or something?
![]() |
Assignee | |
Comment 39•6 years ago
|
||
> Could we disable the check only if we're building Thunderbird? Set a define
> in moz.build based on the value of MOZ_BUILD_APP or something?
An "is this Firefox?" check would be safer than "is this Thunderbird?", because of Seamonkey and possibly other products.
Even then, it's possible that people who distribute Firefox (e.g. Linux packagers) would add their own .js files containing pref overrides. Maybe an NS_ERROR doesn't matter to them; I'm not sure.
Updated•6 years ago
|
![]() |
||
Comment 41•6 years ago
|
||
Should we add a deprecation comment to Add*VarCache functions and/or post on .platform about this new way of doing things?
![]() |
||
Comment 42•6 years ago
|
||
And just to make sure I understand how this works, can we still ship pref overrides to the static prefs without shipping a new binary, via whatever mechanism we have for deploying pref flips on release if needed?
Flags: needinfo?(n.nethercote)
![]() |
Assignee | |
Comment 43•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41) > Should we add a deprecation comment to Add*VarCache functions and/or post on > .platform about this new way of doing things? Probably. I've been meaning to file a bunch of bugs about converting the existing Add*VarCache calls. > And just to make sure I understand how this works, can we still ship pref > overrides to the static prefs without shipping a new binary, via whatever > mechanism we have for deploying pref flips on release if needed? Yes. If a pref value is given in one of the data files (e.g. all.js) that will override the statically-provided value.(Thunderbird relies on this.)
Flags: needinfo?(n.nethercote)
You need to log in
before you can comment on or make changes to this bug.
Description
•