Closed Bug 1436655 Opened 2 years ago Closed 2 years ago

Introduce a mechanism for prefs to be defined entirely in the binary

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

I think this will improve things greatly.
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 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 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+
Blocks: 1436916
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 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 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+
> > +#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?
(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 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 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 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.
Attachment #8949290 - Flags: feedback?(erahm) → feedback+
> > +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.
> 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.
> 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 :)
(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.
well, except if you do char bar[] = "bar", because then you don't get a relocation at all.
(except you can't reassign to that, obviously)
Attachment #8949290 - Flags: feedback?(mh+mozilla) → feedback+
Blocks: 1437168
Depends on: 1436911
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 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+
>  - 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.
Blocks: 1448219
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 :)
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
(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 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 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 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+
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
(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
> > - 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: nobody → n.nethercote
(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 :(
Depends on: 1449143
(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?
> 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.
Duplicate of this bug: 1346224
Should we add a deprecation comment to Add*VarCache functions and/or post on .platform about this new way of doing things?
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)
(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.