Closed Bug 1379383 Opened 8 years ago Closed 7 years ago

Rewrite libpref

Categories

(Core :: Preferences: Backend, enhancement, P3)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

libpref has significant issues. It's an overly-general key/value store, which means pref lookups are relatively slow. Also, lookups can also only happen on the main thread. Both internal and external caching of prefs is used to get around these two issues, which introduces its own problems. Since all prefs are known in advance -- assuming that nobody uses prefs as an ad hoc data store, which we can rely on once 57 blocks XUL add-ons -- they should be stored in a fixed array. This will make lookups fast. Off-main-thread lookups should also be possible. It makes sense for this rewrite to be in Rust, given that libpref is relatively standalone code. I want to take on this rewrite myself, but it's unlikely to happen until Q4.
(In reply to Nicholas Nethercote [:njn] from comment #0) > Since all prefs are known in advance -- assuming that nobody uses prefs as > an ad hoc data store, which we can rely on once 57 blocks XUL add-ons I'm a little dubious about that -- I think datareporting, Log.jsm, urlclassifier, fonts, the IDN whitelist, and some other places use computed keys (or at least created-later keys) into prefs. It's not just add-ons. Furthermore, prefs sync relies on Firefox being able to store arbitrary preferences, because we might be handed key/value pairs that were set by a future version of Firefox. You might be buying into a nine-month multi-feature development cycle here! I'm very interested in this problem, but I think it might need to be tackled more holistically. Accessing prefs from outside Gecko (as we do on Android, coordinating Android SharedPrefs and Gecko's own, and as we will likely do from Rust background services) is tricky. That might mean we build two stores: one being prefs as it is now (with app/distro/locale overlays), and one being a key-value store that's external to Gecko to which we can shunt the other kinds of consumers.
Interesting! Thank you for the info. It sound like a key part of this will be to define exactly what a pref is.
Priority: -- → P3
One pattern to consider for prefs (and eventually other data store bits, when expanded a little): > subscribePref('my pref name', (val) => doSomethingWithNewValue) this would always fire at least once, getting the pref asynchronously, through whatever layers of caching and cache coherency become interesting, and would re-fire whenever the pref changed. Goals should include: - Safe access from multiple threads or processes - Compatibility with sync - Access from JS, Rust, and C++ - Improve perceived performance, particularly at startup - Normalize all pref access, avoiding custom caching, etc - Decide whether this is one piece of a consolidated approach to data
Another interesting pref wrinkle: when a pref is updated in the parent process, messages get sent to the child processes to let them know. To reduce the number of these messages, bug 1376864 introduced an ad hoc, string-matching blacklist of frequently-sent preferences that child processes in practise don't need to know about.
(In reply to Nicholas Nethercote [:njn] from comment #0) > Since all prefs are known in advance -- assuming that nobody uses prefs as > an ad hoc data store, which we can rely on once 57 blocks XUL add-ons -- > they should be stored in a fixed array. This will make lookups fast. > Off-main-thread lookups should also be possible. (In reply to Richard Newman [:rnewman] from comment #1) > Furthermore, prefs sync relies on Firefox being able to store arbitrary > preferences, because we might be handed key/value pairs that were set by a > future version of Firefox. Perhaps there's an opportunity here for "compiled" prefs to be efficient and for "runtime" prefs to be inefficient, providing performance where it's needed most (compiled prefs). HPACK [1] mixes "efficient-lookup" known-likely values with "permit-anything" uncommon strings in a compression-efficient manner. I'm going out on a limb drawing a comparison between compression-efficient and access-efficient, but conceptually there's value in it, and HPACK is used widely in HTTP/2 today. [1] https://tools.ietf.org/html/rfc7541
My main concerns around prefs are: (a) synchronous API. (b) the implementation of a synchronous API via a delayed write-back cache. Consumers act as if prefs is durable storage, when it most certainly is not. (c) lack of common change-management primitives like CAS, builder patterns, or transactions. Having enough runtime information to know who cares about which prefs would definitely be nice to have. I'd be very happy to see prefs split into at least two or three things: - an asynchronous, durable key-value store, which meets the needs of app features looking for simple storage - potentially more complex if Sync is involved (e.g., needs transactions, change counters, versioning). - an asynchronous, durable fixed-prefs store, used for "real prefs". - a read-only synchronous prefs provider, used for fixed settings, the kinds of things we change by making a commit to a source repo. On most platforms we do that via something like AppConstants.swift, and perhaps that's enough. The list of stuff we store in prefs is pretty horrifying: https://github.com/mozilla/firefox-data-store-docs/blob/master/README.md#appendix-1---desktop-prefs
(In reply to Richard Newman [:rnewman] from comment #6) > My main concerns around prefs are: > > (a) synchronous API. FWIW in profiles the thing that shows up when retrieving preferences in practice is the hashtable lookup here: <https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/modules/libpref/prefapi.cpp#748>. Other than that the synchronous API works perfectly fine (as in, I have _never_ seen anything else under our pref getter APIs show up in a profile). Note that this hashtable lookup being expensive is not at all specific to libpref, it's a general issue that affects all PLDHashTable lookups for our large hashtables. We have ongoing work to address these deficiencies, part of it hopefully for 57 and will be doing more work on PLDHashTable performance afterwards. (Note that if we rewrite this code with a very similar scheme in Rust for example which uses a more efficient hashtable implementation based on robinhood hashing -- which is what we're probably going to switch PLDHashTable to use as well) this problem will sort of disappear on its own. Is there any other reason why a synchronous API is considered problematic here? It is quite desirable from a consumer perspective... > (b) the implementation of a synchronous API via a delayed write-back cache. > Consumers act as if prefs is durable storage, when it most certainly is not. I think we really need to define the goals here. For example, please see the work done in bug 789945. As things stand now, we have a setup that works pretty well for application preferences. It is true that the prefs service is currently abused to store things that aren't prefs, and some consumers may want durable storage, but with legacy extensions now almost behind us, we finally are in control of all ends here, so an alternative strategy could be porting the consumers who expect the prefs service to be something that it isn't to use an alternative more sophisticated storage backend. Persisting the prefs DB when prefs are set also has a perf drawback, and given the prevalence of the calls to pref setting APIs across our code base, clearly none of our code currently expects any actual writes to disk happen as a result of such a call. Therefore I think turning the prefs service into a durable storage needs some evidence about the desirability and feasibility of doing so without causing performance regressions and the bar for that should be pretty high. :-) > (c) lack of common change-management primitives like CAS, builder patterns, > or transactions. I'm not sure I understand this part, do you mind elaborating please?
Flags: needinfo?(rnewman)
Pet peeve: The way we do localized prefs and prefs with localized defaults is horrible. We should probably have a constructive pattern for this, but the status quo is a counter example ;-)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7) > FWIW in profiles the thing that shows up when retrieving preferences in > practice is the hashtable lookup here: Perhaps I wasn't clear. The issue for me is not performance. My issue with having a synchronous API is that there's a huge mismatch between the API and its backing storage, and a consequent mismatch between its appearance and its guarantees. Primarily that's about writes, not reads. That prefs is backed by a hashtable that is later maybe flushed to disk, implementing a synchronous write-back cache, and that prefs is used as storage by dozens of platform and frontend features, is not a good combination, regardless of whether that hashtable lookup appears in profiles. > porting the consumers who expect the prefs service to be > something that it isn't to use an alternative more sophisticated storage > backend. Yup. > Persisting the prefs DB when prefs are set also has a perf > drawback, and given the prevalence of the calls to pref setting APIs across > our code base, clearly none of our code currently expects any actual writes > to disk happen as a result of such a call. That's not true: there's plenty of code that expects data written to prefs to not be rolled back to an earlier state (either the previous state or, worse, the state after the last shutdown!) after the write returns. I've personally fixed bugs caused by this assumption. E.g., locale-switching code on Android, which forces a prefs flush to make the system behave correctly. (Things might be even worse on Android because "shutdown" ~= `kill -9`.) Sync expects the lifecycle prefs it writes to persist, because they need to agree with databases. And I'm sure there's more; the codebase has quite a few instances of `savePrefFile(null)`. The majority of engineers I've talked to were unaware that pref changes (and login changes!) could be lost after the write completes. Prefs, like many of our other APIs, give the impression of being rock-solid "old code". > Therefore I think turning the prefs service into a durable storage needs > some evidence about the desirability and feasibility of doing so without > causing performance regressions and the bar for that should be pretty high. > :-) See my point about splitting prefs into two or three things. I'm sure there's code in the codebase that wants to opt in to non-durable storage, and I'm sure there's code that requires synchronous reads, and there might even be code that requires synchronous writes. > > (c) lack of common change-management primitives like CAS, builder patterns, > > or transactions. > > I'm not sure I understand this part, do you mind elaborating please? Anywhere in the codebase that we mutate multiple prefs sequentially and yield the event loop can suffer from consistency issues. Anywhere two components share prefs can suffer from isolation or consistency issues. Prefs as it stands is not a good tool for these cases. For instance, clients sync involves writing three separate values to prefs. If the prefs file is flushed during a sync, and then Firefox doesn't shutdown cleanly, the persisted state will be half old and half new -- the change is not atomic. If two components share two or more values, and use only observers (or no mechanism at all) for coordination, they can see each other's partial writes. If two components are racing to update a pref value, they have no coordination primitive like compare-and-set to avoid error. In short: the prefs API is really only good for settings, and many of the 1200+ prefs written in Firefox should be using something else. By contrast, the Android `SharedPreferences` API has an atomic builder pattern (`prefs.edit().putInt("a", 5).putInt("b", 6).apply()`), offers both synchronous and asynchronous writes (`apply` vs `commit`), and has OS support to guarantee that writes make it into the backing XML file even if the application crashes. It's still not all the way to a transactional key-value store, but it's better than libpref.
Flags: needinfo?(rnewman)
(In reply to Richard Newman [:rnewman] from comment #9) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #7) > > > FWIW in profiles the thing that shows up when retrieving preferences in > > practice is the hashtable lookup here: > > Perhaps I wasn't clear. The issue for me is not performance. My issue with > having a synchronous API is that there's a huge mismatch between the API and > its backing storage, and a consequent mismatch between its appearance and > its guarantees. Primarily that's about writes, not reads. > > That prefs is backed by a hashtable that is later maybe flushed to disk, > implementing a synchronous write-back cache, and that prefs is used as > storage by dozens of platform and frontend features, is not a good > combination, regardless of whether that hashtable lookup appears in profiles. As far as I know, the prefs file is flushed to disk at shutdown time, not upon writing to it using the prefs API to set prefs... And I was arguing that is a desirable feature, and we shouldn't switch to writing the file upon setting prefs. Agreed that sometimes some code uses prefs as a storage mechanism but I view that as abusing prefs for something that it wasn't intended to do, and I'd much rather have such code move to a proper storage backend than making libpref be that backend. > > Persisting the prefs DB when prefs are set also has a perf > > drawback, and given the prevalence of the calls to pref setting APIs across > > our code base, clearly none of our code currently expects any actual writes > > to disk happen as a result of such a call. > > That's not true: there's plenty of code that expects data written to prefs > to not be rolled back to an earlier state (either the previous state or, > worse, the state after the last shutdown!) after the write returns. Perhaps I wasn't clear. I didn't argue that you can't find examples of code which does want the result of a pref setting API to be persisted to disk, that's obviously not the case. My point was that there are a lot of existing consumers of pref setting APIs that do not expect there to be an IO intensive performance penalty to be associated with a call to a pref setter API. > I've personally fixed bugs caused by this assumption. E.g., locale-switching > code on Android, which forces a prefs flush to make the system behave > correctly. (Things might be even worse on Android because "shutdown" ~= > `kill -9`.) > > Sync expects the lifecycle prefs it writes to persist, because they need to > agree with databases. And I'm sure there's more; the codebase has quite a > few instances of `savePrefFile(null)`. I see about 50 or so, with ~10 from tests based on a very quick skim: <https://searchfox.org/mozilla-central/search?q=symbol:_ZN14nsIPrefService12SavePrefFileEP7nsIFile%2C%23savePrefFile&redirect=false> Again, I'm not saying that there are cases where we do want to persist the prefs file to disk immediately after setting a pref, but the question is whether it makes sense to do that after setting each and every pref. I still don't think the current usage of the prefs API suggests that the API is used expecting such persistence guarantees. > The majority of engineers I've talked to were unaware that pref changes (and > login changes!) could be lost after the write completes. Prefs, like many of > our other APIs, give the impression of being rock-solid "old code". That's a problem, but perhaps it could be addressed through better documentation. > > Therefore I think turning the prefs service into a durable storage needs > > some evidence about the desirability and feasibility of doing so without > > causing performance regressions and the bar for that should be pretty high. > > :-) > > See my point about splitting prefs into two or three things. I'm sure > there's code in the codebase that wants to opt in to non-durable storage, > and I'm sure there's code that requires synchronous reads, and there might > even be code that requires synchronous writes. To clarify comment 7, I never meant to suggest that we should have an API for synchronously reading/writing anything from/to the disk, I think we have a lot of practical experience to suggest that will result in extremely poor performance. My suggestion there was based on the current design of the preferences module specifically with having an in-memory table of preferences that the read APIs query. With that design, a synchronous API is superior because it is easier to use by consumers (assuming that we can fix the performance problem I mentioned before.) > > > (c) lack of common change-management primitives like CAS, builder patterns, > > > or transactions. > > > > I'm not sure I understand this part, do you mind elaborating please? > > Anywhere in the codebase that we mutate multiple prefs sequentially and > yield the event loop can suffer from consistency issues. Anywhere two > components share prefs can suffer from isolation or consistency issues. > Prefs as it stands is not a good tool for these cases. > > For instance, clients sync involves writing three separate values to prefs. > If the prefs file is flushed during a sync, and then Firefox doesn't > shutdown cleanly, the persisted state will be half old and half new -- the > change is not atomic. Hmm, perhaps my reading of the code is incorrect, but I'm not quite sure how this can happen. The preferences are written using a safe output stream which does the write atomically using a temporary file as the output buffer <https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/modules/libpref/Preferences.cpp#279>, so in the scenario above, if Firefox doesn't shutdown cleanly, the prefs file will remain untouched. > If two components share two or more values, and use only observers (or no > mechanism at all) for coordination, they can see each other's partial writes. > > If two components are racing to update a pref value, they have no > coordination primitive like compare-and-set to avoid error. In addition to the above mechanism, we also have sPendingWriteData <https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/modules/libpref/Preferences.cpp#357> to deal with multiple pref write runnables that may be in-flight at a time. I'm fairly sure that at least in the trivial cases libpref handles all of the easy cases of atomicity fine. Not claiming that the current code is bug free of course, but I would be surprised if we can't get the most basic scenarios wrong. > In short: the prefs API is really only good for settings, and many of the > 1200+ prefs written in Firefox should be using something else. > > By contrast, the Android `SharedPreferences` API has an atomic builder > pattern (`prefs.edit().putInt("a", 5).putInt("b", 6).apply()`), offers both > synchronous and asynchronous writes (`apply` vs `commit`), and has OS > support to guarantee that writes make it into the backing XML file even if > the application crashes. It's still not all the way to a transactional > key-value store, but it's better than libpref. FWIW libpref also has an API to guarantee saving of the pref file <https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/modules/libpref/Preferences.h#391> which is currently exposed to C++ only (but could be exposed to JS as well if needed.) I'm not familiar with Android's SharedPreferences API so I'm not going to compare the two APIs.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10) > As far as I know, the prefs file is flushed to disk at shutdown time, not > upon writing to it using the prefs API to set prefs... Indeed -- that's what I meant by "later maybe flushed to disk". > Agreed that sometimes some code uses prefs as a storage mechanism but I view > that as abusing prefs for something that it wasn't intended to do, and I'd > much rather have such code move to a proper storage backend than making > libpref be that backend. I think we are agreed. Use libpref for data that it's acceptable to lose routinely on crash, and that is OK to be persisted at any intermediate in-memory state -- typically independent values -- and otherwise use something else. Unfortunately, prefs is used as backing storage to varying extents by the updater, migration flags, safe browsing, Pocket, data reporting, blocklist, Kinto, and Sync… and perhaps others, depending on where you draw the line between settings that are OK to lose and component or service-client data that needs to stick around. > My point was that there are a lot of > existing consumers of pref setting APIs that do not expect there to be an IO > intensive performance penalty to be associated with a call to a pref setter > API. I see what you mean. But those consumers can't have their cake and eat it: if they expect their writes to persist, and also expect no proximal IO, then they're going to be disappointed somewhere. The current API disappoints them wrt durability, which is something few engineers seem to consider. > Hmm, perhaps my reading of the code is incorrect, but I'm not quite sure how > this can happen. The preferences are written using a safe output stream… Here's a very simple example. It's a fair ways from the real world -- this simple scenario could be fixed in code -- but it illustrates the point. Say I have two prefs: `foo.start` and `foo.end`. My code, quite reasonably, expects the invariant `foo.end` >= `foo.start`. The starting values in prefs.js are 100 and 200. My code then does something straightforward like: ``` // BEGIN TRANSACTION, if we could setPref("foo.start", Date.now()); try { // X <<<< await foo.act(); } finally { // Y <<<< // COMMIT setPref("foo.end", Date.now()); } // END TRANSACTION ``` Under normal operation this behaves as desired: on shutdown `foo.start` and `foo.end` are both advanced. However, under real-world conditions this will exhibit bugs. If at point "X" or "Y" some code causes a prefs flush to occur, then entirely unknown to my code the disk state will have `foo.start > foo.end`. If the code continues to happily run to completion, but Firefox crashes before the prefs file is again flushed, then on relaunch `foo.start > foo.end` -- we partly rolled back to the inconsistent disk state! If, on the other hand, nothing flushes prefs at all before the crash, then history is rewound entirely: on relaunch start = 100, end = 200, as if the `setPref` calls never ran, which will probably disagree significantly with any side effects in `foo.act()`. These are not desirable properties, and I'll wager that most of our engineers do not consider these possibilities when cranking out frontend code that persists state. None of the outcomes above will be caught by normal automated testing -- we don't have a Chaos Monkey to verify that all Firefox features are internally consistent (and consistent with network services!) when the browser crashes at random points. Few situations like this will be caught by code review. Prefs is sadly a widespread, well-known, and deeply deceptive API. > I'm fairly sure that at least in the trivial cases libpref handles all of > the easy cases of atomicity fine. I think you're using "atomicity" at a different level of abstraction to me. I don't mean consistent updates of in-memory data structures, though I'm glad we do that correctly! I mean atomicity in the database sense: that in a given collection of writes (a transaction), either all succeed or none do. If I have two prefs writers on different conceptual threads (either real threads or interleaved on the JS event loop), doing: ``` A: x = 5 A: y = 2 B: x = 4 B: y = 3 ``` or similar, libpref has no mechanisms for assuring that `(x, y)` is _either_ `(5, 2)` _or_ `(4, 3)`, and never `(5, 3)` or `(4, 2)`. Furthermore, AFAIK it does not report to either A or B that their writes collided. A step beyond this is isolation, in which B could safely do `y = x + 2` and always be guaranteed that `y` would be 6, not 7. If I'm missing some parts of the code that implement isolation or batched/atomic changes, please point me to them! > FWIW libpref also has an API to guarantee saving of the pref file > <https://searchfox.org/mozilla-central/rev/ > 18c16ebf818abb86805ce08a6e537e4cd826f044/modules/libpref/Preferences.h#391> > which is currently exposed to C++ only (but could be exposed to JS as well > if needed.) JS code mostly just calls `savePrefFile()` and hopes for the best, which in real world use is likely to be acceptable, given libpref's other limitations. Given that on my profile that flushes 100KB to disk each time it's called, it's not really a good routine option for durability.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10) > As far as I know, the prefs file is flushed to disk at shutdown time, not > upon writing to it using the prefs API to set prefs... And I was arguing > that is a desirable feature, and we shouldn't switch to writing the file > upon setting prefs. And I would oppose this. I find any software that stores its preferences only at clean shutdown incredibly stupid. When we crash, we loose the settings completely. Some lazy delayed aggregated write should be there. I don't think flushing it every minute or two would be that bad...
The thread restrictions just bit me in bug 1375978, where I'm moving font enumeration off the main thread. Enumeration on Mac reads preferences while enumerating fonts under some circumstances, so I'm having to refactor font initialization over in bug 1395061 to avoid accessing prefs during enumeration.
(In reply to Richard Newman [:rnewman] from comment #11) > > Hmm, perhaps my reading of the code is incorrect, but I'm not quite sure how > > this can happen. The preferences are written using a safe output stream… > > Here's a very simple example. It's a fair ways from the real world -- this > simple scenario could be fixed in code -- but it illustrates the point. > > Say I have two prefs: `foo.start` and `foo.end`. > > My code, quite reasonably, expects the invariant `foo.end` >= `foo.start`. OK, so with the current API, I'll edit your pseudo code to show how it should be fixed using the current API correctly. I'd argue that if this is the desired invariant, this code is just using the current API incorrectly. > ``` > // BEGIN TRANSACTION, if we could > setPref("foo.start", Date.now()); > try { > // X <<<< > await foo.act(); > } finally { > // Y <<<< > // COMMIT > setPref("foo.end", Date.now()); savePrefFile(null); > } > // END TRANSACTION > ``` > > Under normal operation this behaves as desired: on shutdown `foo.start` and > `foo.end` are both advanced. With my modification above, this invariant is now preserved both in the case of a clean shutdown and in the case of a crash sometime after the transaction is committed. Since this is a fake transaction, it's unreasonable to expect a crash after setting foo.start but before foo.end to leave things in a consistent state, but that's OK since this code also preserves that invariant. > These are not desirable properties, and I'll wager that most of our > engineers do not consider these possibilities when cranking out frontend > code that persists state. Hmm, front-end code only? Perhaps, I'm not sure. Again, I'm not sure. But I still don't understand how much of this is an issue of not knowing how to use the current API vs the fault of the current API. I feel like I don't have much else to add to convince you on top of what I have said before. > None of the outcomes above will be caught by normal automated testing -- we > don't have a Chaos Monkey to verify that all Firefox features are internally > consistent (and consistent with network services!) when the browser crashes > at random points. Few situations like this will be caught by code review. > Prefs is sadly a widespread, well-known, and deeply deceptive API. We actually do have the mechanism to do that kind of testing if that's needed, FWIW. We have ChaosMode.h, it would be quite easy to add a ChaosFeature to it that would create a chaos level that would randomly force-save the prefs file after a call and or introduce a crash after one such call if such testing is desirable. > > I'm fairly sure that at least in the trivial cases libpref handles all of > > the easy cases of atomicity fine. > > I think you're using "atomicity" at a different level of abstraction to me. > I don't mean consistent updates of in-memory data structures, though I'm > glad we do that correctly! I mean atomicity in the database sense: that in a > given collection of writes (a transaction), either all succeed or none do. No, I also meant atomicity in the sense of what gets written to the disk. > If I have two prefs writers on different conceptual threads (either real > threads or interleaved on the JS event loop), doing: The prefs service is main thread only and can't be accessed off the main thread. I'm not sure what you mean by interleaved execution on the JS event loop. As far as I can tell concurrent calls like this can't really happen. > ``` > A: x = 5 > A: y = 2 > B: x = 4 > B: y = 3 > ``` > > or similar, libpref has no mechanisms for assuring that `(x, y)` is _either_ > `(5, 2)` _or_ `(4, 3)`, and never `(5, 3)` or `(4, 2)`. > > Furthermore, AFAIK it does not report to either A or B that their writes > collided. > > A step beyond this is isolation, in which B could safely do `y = x + 2` and > always be guaranteed that `y` would be 6, not 7. > > If I'm missing some parts of the code that implement isolation or > batched/atomic changes, please point me to them! SavePrefFile() issues an async save: <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/modules/libpref/Preferences.cpp#935> which gets to WritePrefFile(): <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/modules/libpref/Preferences.cpp#1200> There, we either dispatch a new runnable to the writer background thread, or if there is a pending write data (aka a write operation from a previous call to SavePrefFile() is currently in progress) we update the data to be written: <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/modules/libpref/Preferences.cpp#1229> The runnable, when called on the background thread, calls PreferenceWriter::Write(): <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/modules/libpref/Preferences.cpp#375> That function writes the file atomically: <https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/modules/libpref/Preferences.cpp#278> This ensures that the data written to the disk is a consistent snapshot of the prefs hashtable according to its state at the *latest* time savePrefFile() was called. > > FWIW libpref also has an API to guarantee saving of the pref file > > <https://searchfox.org/mozilla-central/rev/ > > 18c16ebf818abb86805ce08a6e537e4cd826f044/modules/libpref/Preferences.h#391> > > which is currently exposed to C++ only (but could be exposed to JS as well > > if needed.) > > JS code mostly just calls `savePrefFile()` and hopes for the best, which in > real world use is likely to be acceptable, given libpref's other > limitations. I hope the above shows that libpref doesn't just hope for the best and actually does a decent job at ensuring that the correct data gets written to the disk. > Given that on my profile that flushes 100KB to disk each time > it's called, it's not really a good routine option for durability. Yes I very much agree. Writing a 100KB file to disk with any regular frequency is not a good option for anything. This is what I have been arguing for all along. :-) (In reply to Honza Bambas (:mayhemer) from comment #12) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #10) > > As far as I know, the prefs file is flushed to disk at shutdown time, not > > upon writing to it using the prefs API to set prefs... And I was arguing > > that is a desirable feature, and we shouldn't switch to writing the file > > upon setting prefs. > > And I would oppose this. I find any software that stores its preferences > only at clean shutdown incredibly stupid. When we crash, we loose the > settings completely. Some lazy delayed aggregated write should be there. I > don't think flushing it every minute or two would be that bad... I don't know how closely you have followed the conversation here, but what I am arguing against is turning the preferences module into something that suddenly does IO per each and every call to any of our pref setter APIs. As far as I can tell, a lazy aggregated pref writer (which would be quite reasonable, and wouldn't require any drastic changes to the current design of the prefs module IMHO) isn't what's being proposed here. I could certainly get behind such a proposal... (In reply to Myk Melez [:myk] [@mykmelez] from comment #13) > The thread restrictions just bit me in bug 1375978, where I'm moving font > enumeration off the main thread. Enumeration on Mac reads preferences while > enumerating fonts under some circumstances, so I'm having to refactor font > initialization over in bug 1395061 to avoid accessing prefs during > enumeration. Yes, the prefs service being main thread only is a super well known limitation of it. For some history, see bug 619487. Also, see WorkerPrefs.h which shows how we have to manually "declare" prefs that need to be accessible on Web Workers. It would certainly be nice to have a nicer story for accessing prefs off the main thread, but we should think very carefully about doing it in a way to avoid causing performance regressions for the main-thread case. Accesses to the prefs service very regularly show up in performance profiles, so any changes to the prefs API can be performance sensitive...
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14) > We actually do have the mechanism to do that kind of testing if that's > needed, FWIW. We have ChaosMode.h, it would be quite easy to add a > ChaosFeature to it that would create a chaos level that would randomly > force-save the prefs file after a call and or introduce a crash after one > such call if such testing is desirable. ++ to this specifically as its own bug, regardless of whatever else comes of this bug.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14) > (In reply to Honza Bambas (:mayhemer) from comment #12) > I don't know how closely you have followed the conversation here, roughly only. > but what I > am arguing against is turning the preferences module into something that > suddenly does IO per each and every call to any of our pref setter APIs. I fully agree. > As > far as I can tell, a lazy aggregated pref writer (which would be quite > reasonable, and wouldn't require any drastic changes to the current design > of the prefs module IMHO) isn't what's being proposed here. I could > certainly get behind such a proposal... I just wanted make sure this (to not write ONLY at shutdown) has been raised and that, when this rewrite is about to happen, we consider a lazy writer (not necessarily in this bug). Thanks.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14) > OK, so with the current API, I'll edit your pseudo code to show how it > should be fixed using the current API correctly. I'd argue that if this is > the desired invariant, this code is just using the current API incorrectly. I think that adding manual `savePrefFile` calls is not a great solution. It's expensive and it's error-prone. The reason I gave this example is that _this is what engineers write_, in my experience. You have a sophisticated mental model of libpref that is not the same as the mental model conveyed by its documentation, nor the mental model I've seen in front-end engineers. For example, `setCharPref`'s docs make absolutely no mention of durability concerns. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#setCharPref() Even `savePrefFile`'s documentation doesn't discuss that it _must_ be called to guarantee that a write has been persisted. libpref is the only disk-backed storage API I can recall in which a write can complete without throwing and minutes later not have made it to disk, with no structural reminder like an extant transaction object or a returned promise or a callback function. That's alien to anyone who hasn't spent ten years with this codebase. > We actually do have the mechanism to do that kind of testing if that's > needed, FWIW. We have ChaosMode.h, it would be quite easy to add a > ChaosFeature to it that would create a chaos level that would randomly > force-save the prefs file after a call and or introduce a crash after one > such call if such testing is desirable. I didn't think ChaosMode was involved in process orchestration or TPS-style testing? > The prefs service is main thread only and can't be accessed off the main > thread. I'm not sure what you mean by interleaved execution on the JS event > loop. I mean we have async calls, observer notifications, timers, and all that fun stuff. It's possible for a linear sequence of code that includes `await` to be interleaved with other code, no? Sync does this, for example: it'll do some work, make an HTTP request or do an async DB write, and when that request or write is done it does some more work. While that request is running we're spinning the event loop, and that can involve writing -- or even reading -- the same prefs that are touched by the async code. > This ensures that the data written to the disk is a consistent snapshot of > the prefs hashtable according to its state at the *latest* time > savePrefFile() was called. Yes, I understand that. I agree that libpref dumps a consistent snapshot of the in-memory state to disk. I'll restate ACID, in brief: Atomicity is the property that all of the writes in a particular transaction occur, or none do. Because libpref doesn't support transactions, it's impossible for it to exhibit this property, as it's commonly understood, without restructuring async application code to put all the writes in the same event loop tick (that is, by avoiding the possibility that other code will cause a flush in the middle of a 'transaction'). Consistency means that invalid data doesn't make it to disk. libpref does a good job of this wrt flushing, and a reasonable job wrt excluding invalid values. Isolation is the property that two separate writers don't see each other's writes within the scope of their own transactions. libpref doesn't have that, because it has no conception of different writers or of transactions. Durability is the property that once a write completes, it won't ordinarily be lost. libpref has this if you immediately and synchronously flush the current state of prefs to disk with `savePrefFile`. Unfortunately, this impacts atomicity. So there are the only two ways to ensure atomicity (that two prefs are both written to disk, or neither are) or isolation with libpref: 1. Don't write to more than one pref. 2. Don't write async code. And there's only one way to ensure durability: 1. Always call `savePrefFile` on the same event loop tick as your writes. That makes libpref well-suited for backing about:config to turn flags on and off -- durability isn't important, flags are independent, and we flush when done. It makes it reasonably well-suited to back Preferences, because again, it doesn't matter too much if data is lost on crash. It makes it poorly suited for programmatic data storage, which has to contend with reading and writing multiple values, interleaving async code with other components, and higher write volumes with durability and performance requirements. Unfortunately, there is a lot of programmatic data storage in Firefox that uses libpref. > Yes I very much agree. Writing a 100KB file to disk with any regular > frequency is not a good option for anything. This is what I have been > arguing for all along. :-) Excellent, we are agreed! 1. libpref's only option for durable writes is savePrefFile. 2. savePrefFile is expensive. => 3. libpref is not suitable for durable data written at any frequency. Right? > I don't know how closely you have followed the conversation here, but what I > am arguing against is turning the preferences module into something that > suddenly does IO per each and every call to any of our pref setter APIs. I'm not arguing that we should _turn_ the prefs module into such a thing. I think that the requirements of simple, quick-to-read flags with weak durability ('settings'), and the requirements of simple storage for application features are in tension: the former is very willing to trade ACID for speed, and the latter expects at least D, and probably the full set, by default. As such, I'm arguing that "rewrite libpref" is really "split libpref into at least two things", and a rewrite of just the current feature set, but aiming for more perf and better thread behavior, won't really fix many of the problems I've described. If we do this _right_, we can resolve the tension between the two kinds of consumers and make both sides better: take advantage of the characteristics of settings-like prefs and exclude the more difficult stuff (e.g., computed pref names, complex prefs, ad hoc types) to make the simple case faster, and in parallel provide a solution for app storage that meets those very different requirements.
Everybody on this conversation is aware that, as of bug 981818, we save the pref file on "every change" - on a timer, for batching purposes. It seemed from some comments that maybe it isn't clear that has recently changed. There are a couple of things that are missing in the prefs, to make user interface nicer - if there is a rewrite going on, keep that in mind? (e.g., I was playing with this in bug 1240198.) * It would be nice if we could avoid reading the all.js file every time. Seems like we could have the initial values compiled in. * While we have a concept of a default value, we don't have a concept of a "Firefox started with this value". * We are not capturing the "this preference needs a restart to make a difference" (e.g., the reason graphics team set up gfxPrefs.) While moving the graphics preferences to that wrapper, I've found a number of places where the code was doing both - on a preference change, some of the code would pick up the new value, some of it would keep using the startup time.
(In reply to Milan Sreckovic [:milan] from comment #18) > Everybody on this conversation is aware that, as of bug 981818, we save the > pref file on "every change" - on a timer, for batching purposes. It seemed > from some comments that maybe it isn't clear that has recently changed. Fantastic! I actually wasn't aware that we made that change. It seems that libpref as it is now has even better durability guarantees than I had realized. Thanks for mentioning this. (In reply to Richard Newman [:rnewman] from comment #17) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #14) > > > OK, so with the current API, I'll edit your pseudo code to show how it > > should be fixed using the current API correctly. I'd argue that if this is > > the desired invariant, this code is just using the current API incorrectly. > > I think that adding manual `savePrefFile` calls is not a great solution. > It's expensive and it's error-prone. Per comment 18, manual savePrefFile calls are only needed now in places where the existing 500ms batching is insufficient, FWIW. > The reason I gave this example is that _this is what engineers write_, in my > experience. You have a sophisticated mental model of libpref that is not the > same as the mental model conveyed by its documentation, nor the mental model > I've seen in front-end engineers. > > For example, `setCharPref`'s docs make absolutely no mention of durability > concerns. > > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/ > Interface/nsIPrefBranch#setCharPref() > > Even `savePrefFile`'s documentation doesn't discuss that it _must_ be called > to guarantee that a write has been persisted. > > libpref is the only disk-backed storage API I can recall in which a write > can complete without throwing and minutes later not have made it to disk, > with no structural reminder like an extant transaction object or a returned > promise or a callback function. That's alien to anyone who hasn't spent ten > years with this codebase. Again, per comment 18 this issue is fixed now. > > We actually do have the mechanism to do that kind of testing if that's > > needed, FWIW. We have ChaosMode.h, it would be quite easy to add a > > ChaosFeature to it that would create a chaos level that would randomly > > force-save the prefs file after a call and or introduce a crash after one > > such call if such testing is desirable. > > I didn't think ChaosMode was involved in process orchestration or TPS-style > testing? Sorry, I'm not sure what those two things are. :-) ChaosMode AFAIK has been used to help reproduce intermittent failures, not sure if it has been used for other purposes so far. > > This ensures that the data written to the disk is a consistent snapshot of > > the prefs hashtable according to its state at the *latest* time > > savePrefFile() was called. > > Yes, I understand that. I agree that libpref dumps a consistent snapshot of > the in-memory state to disk. > > I'll restate ACID, in brief: [snip] > Unfortunately, there is a lot of programmatic data storage in Firefox that > uses libpref. I understand what ACID means, but I think we're talking in circles now. As I mentioned in comment 7, I think the use case of storing application preferences and other data storage needs which require more sophisticated features such as ACID guarantees are different. I think perhaps this is the core part of our disagreement. I'd be happy to let Nick make a decision as the module owner. To be honest, I prefer to see a better description of the set of problems that we would like to solve with the rewrite of libpref first, and have people agree that those are problems worth solving before spending too much time discussing the specific solutions we pick. I think some of our current disagreements are coming from having a different set of problems in mind when thinking about this. > > Yes I very much agree. Writing a 100KB file to disk with any regular > > frequency is not a good option for anything. This is what I have been > > arguing for all along. :-) > > Excellent, we are agreed! > > 1. libpref's only option for durable writes is savePrefFile. > 2. savePrefFile is expensive. > => > 3. libpref is not suitable for durable data written at any frequency. > > Right? Just to ensure that what I said is understood correctly, I meant that the current format we store the file in makes it impossible for us to update only part of it when a single or a few preferences change, so it forces us to write the entire flat file every time, which is perfectly fine if the file fits in one page of the disk most of the time but not historically as the size of the prefs file can get quite large (for example hundreds of KB). But the reason the size of the file can get so large is that the prefs DB has historically been abused as a data storage service by some code, sometimes our code, sometimes add-on code. The add-on part of the problem will no longer be an issue post 57, so the landscape is changing, even though users with existing profiles with large prefs files are stuck with them. There are also good reasons to keep the current file format to consider, such as keeping it readable and editable by users. But even then we could probably look into doing more sophisticated things such as in place updating things if the size of the new entry is smaller than the old entry. But we should probably start by getting some telemetry on the size of the prefs file in the wild if we don't have one to see what the distribution among our user base looks like before deciding how to attack that problem should we decide to do so IMO.
Are users stuck with large preference files because of an unfixable reason, or is it because we haven’t written a purge for the now-invalid preferences yet due to priority/time constraints?
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #19) > Per comment 18, manual savePrefFile calls are only needed now in places > where the existing 500ms batching is insufficient, FWIW. Which is great! That's just a rounding error away from durability, which is the best we can hope for. > > I didn't think ChaosMode was involved in process orchestration or TPS-style > > testing? > > Sorry, I'm not sure what those two things are. :-) ChaosMode AFAIK has > been used to help reproduce intermittent failures, not sure if it has been > used for other purposes so far. I mean tests that span multiple invocations of Firefox, perhaps interacting with a remote service. E.g., "no matter how many times, or when, we crash Firefox, Sync always ends up downloading the correct data". IIRC TPS uses mozrunner to do a non-chaos version of this. > I think the use case of storing application preferences and other data storage > needs which require more sophisticated features such as ACID guarantees are > different. I think perhaps this is the core part of our disagreement. On the contrary, I think we're exactly agreeing: libpref is not a good place to put many of the things that live there -- for size reasons, for churn reasons, and indeed for sophistication reasons -- and everyone's lives are better if we find a way to only use it for application preferences. Probably that means making a better place, solving migration, and then making libpref gently unwelcoming to the wrong kinds of uses so it can focus on being fast for the others.
(In reply to Richard Soderberg [:atoll] from comment #20) > Are users stuck with large preference files because of an unfixable reason, > or is it because we haven’t written a purge for the now-invalid preferences > yet due to priority/time constraints? Perhaps a little bit of both. For one thing, there's no easy way to tell whether a preference is being used by any code or not at the moment (we don't have a grand central list of all of the preferences that our code expects to exist in all configurations.) So it's not easy to look at an existing file and decide which parts of it to purge.
That sounds entirely priority/business dependent, since as of 57 we control all such possible locations and thus have the technical capability to - but, not yet, a reason to - audit all prefs. Since such an audit will need to occur someday to find all of the libpref uses and evaluate whether they need prefs, or something else (see many above), I’m mostly checking to make sure that the only actual hard stop obstacle is importance.
There's something of a summary about this stuff here: https://docs.google.com/document/d/1-N0vd07SW8-FTfZdcR7SI4D2udnWE1Bs9lgNadFmc-Q/edit#heading=h.1umaq7qmh1ow My plan is to look into this in detail in Q4. I will analyze the discussion above, as well as the code in its current state. Once I have a solid understanding of that, then I will start thinking about a new design.
For those who are interested, bug 1407526 is where I am tracking clean-ups I'm making to the Preferences code. Why clean up the code of a module that may be rewritten, you may wonder? Because that's how I learn about code; it's a combined clean-up + familiarization exercise. And if the module doesn't end up being rewritten, at least it'll end up in better shape.
I'm going to close this bug. Although I am doing significant work on libpref (e.g. see bug 1407526) I am not going to do a full rewrite in either Rust or C++, because that doesn't make sense. There are enough consumers of libpref and constraints on it -- comment 0 was naive! -- that evolution makes a lot more sense than outright replacement. (Rust fans should note that I rewrote the parser in Rust, though, in bug 1423840 :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.