160377 - Prefs should have a dirty flag to avoid redundant prefs file saves

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect, P2)

Description

[Simon Fraser [no longer active]]

There's no way at the moment to tell if the prefs have been changed since the
last save to disk, so that we can't avoid doing redundant, and expensive file saves.

It would be useful to have something on the pref API that allows embedders to
tell if the prefs are dirty. Furthermore, perhaps a 'mod count' or numeric value
could be used so that callers can tell if the prefs have changed since some
previous time. Implementation would just incrememnt this value on every pref
change, and set it to 0 on successful file save.

Comment 1

[Jeremy M. Dolan]

I think there is a bug elsewhere about keeping track of all profile data
dirtyness, so laptops aren't constantly woken up. It was less about the
'expensiveness' and more about timing writes together when the drive is spinning
anyway, based on user activity.

Comment 2

[Alec Flett]

Attachment: Add a "dirty" attribute

This patch adds a new interface, nsIPrefService, which keeps track of a global
flag, gDirty, which knows whether or not the pref service is dirty or not. 

I didn't like making the flag global, but prefs is such a mess that I needed to
be able to get to it from both prefapi.cpp and nsPrefService.

in any case, I also added a warning for when we write out prefs redundantly. I
didn't want to actually prevent it from writing out the file just yet because I
think there may be strange cases that we aren't aware of just yet. I'd like to
run with this and see what happens in the comercial builds, etc..

Comment 3

[Brian Nesse (gone)]

You are setting gDirty at the end of pref_HashPref, this will cause the state to
set dirty regardless of whether a change occured or not. It also seems redundant
as you are also setting it in pref_SetValue (which seems like the correct place
to do it.)

PREF_ClearUserPref is never setting the flag, and PREF_ClearAllUserPrefs is
setting it whether it clears anything or not (might be better to do it in the
callback, but I suppose it may not be that important).

PREF_DeleteBranch might also set the flag when nothing is deleted, but I could
live with that.

Comment 4

[Alec Flett]

mozilla 1.2alpha is more or less done. moving mozilla 1.2alpha bugs out to
mozilla 1.2beta

Comment 5

[Alec Flett]

These bugs missed the 1.2 train. Moving these out to 1.3alpha

Comment 6

[Alec Flett]

moving somewhat important bugs to 1.3beta for consideration - some of these may
be bumped out to 1.4alpha after being evaluated.

Comment 7

[Alec Flett]

Attachment: Add a "dirty" attribute, v1.1

this addresses brian's comments:
- gDirty is now only set when pref_HashPref results in the value changing (this
also means that is is not set when you set a pref to its current value, which
is nice)
- the default prefs file is not written when gDirty is false.

Comment 8

[Alec Flett]

Comment on attachment 111642 [details] [diff] [review]
Add a "dirty" attribute, v1.1

timeless/dveditz, can I get a review on this? This should help some performance
stuff, including shutdown performance since we write the prefs file out some 4
times during shutdown...

Comment 9

[timeless]

Comment on attachment 111642 [details] [diff] [review]
Add a "dirty" attribute, v1.1

new files should be MPL/...

+   * is the pref store dirty, and possibly needs to
+   * be written to disk?
perhaps:
* Whether the pref store is dirty and therefore
* needs to be written to disk.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 111642 [details] [diff] [review]
Add a "dirty" attribute, v1.1

>+                    <PATH>nsIPrefServiceInternal.idl</PATH>

Have we finally givenup on "nsPI..." names for internal interfaces?

>diff -N public/nsIPrefServiceInternal.idl

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Should be the *Mozilla* PL, not Netscape, for new files. Unless you're saying
this is a derivative ("Modification") of an existing file. Couple other updates
in the license block follow from that. Whether you change the license or not
you need to update the copyright date which shouldn't be 1998 for a new file.

>+ * Contributor(s):
>+ * Alec Flett <[e-mail]>
>+ * Brian Nesse <[e-mail]>

Did bnesse contribute to this new file?

>+[scriptable, uuid(79A55A09-713E-42d5-B983-5228FF501118)]
>+interface nsIPrefServiceInternal : nsISupports
>+{
>+  /**
>+   * is the pref store dirty, and possibly needs to
>+   * be written to disk?
>+   */
>+  readonly attribute boolean dirty;
>+  
>+};

I didn't see where this interface was used, looks like you use the internal
global gDirty instead (which is less overhead anyway). Can we get rid of this
interface and makefile changes?

Comment 11

[Alec Flett]

I'll fix the license issues

but as for this new attribute, this was a request by chimera (see the top of the
bug)

Simon, how critical is it for you to have the 'dirty' attribute? We could just
hide it all inside libpref..

Comment 12

[Simon Fraser [no longer active]]

It would be fine to hide it inside libpref, but then what does the SavePrefFile
call do? Does it do nothing, or does it save anyway? Maybe SavePrefFile needs a
param to say 'really save'?

Comment 13

[Daniel Veditz [:dveditz]]

Why would you care if SavePrefs does nothing if there's really nothing to do?

Anyway, if you need the property then fine, but since I didn't see it used I
worried it would just be bloat (another .xpt loaded in memory everytime, etc).

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 111642 [details] [diff] [review]
Add a "dirty" attribute, v1.1

sr=dveditz though I'd prefer skipping the extra idl and related support until
someone demonstrates a need for it (such as linking in a bug showing a patch
ready to use it to good effect?)

Comment 15

[Alec Flett]

ok, I landed the patch without the nsIPrefServiceInternal stuff

as for forcing the write to disk, I'm not actually sure I see the advantage of
being able to do that in an API. Besides, nsIPrefService is frozen (Which was
the whole reason for the "Internal" version anyway)