PDA

View Full Version : Prefs thought.



Dan Sully
2005-07-21, 16:05
Here's a quick patch that will only write out prefs if a value has changed.

I'd like to use this to get rid of the scheduledWrite code, which has been
causing support isses.

Thoughts?

-D
--
Do not panic, do not panic! We are trained professionals!
Now, stay calm. We are going around the leaf.

Grotus
2005-07-21, 16:16
It's never going to hit the case where $oldValue eq $value, since that
possibility is precluded by all the checks made prior to setting the
preference (see all those returns you shifted left).

Hopefully the r3770 change I just put in will help. It writes out the
prefs file in the cleanup code if there is a write pending.


Dan Sully wrote:
> Here's a quick patch that will only write out prefs if a value has changed.
>
> I'd like to use this to get rid of the scheduledWrite code, which has been
> causing support isses.
>
> Thoughts?
>
> -D
>
>
> ------------------------------------------------------------------------
>
> Index: Slim/Utils/Prefs.pm
> ================================================== =================
> --- Slim/Utils/Prefs.pm (revision 3769)
> +++ Slim/Utils/Prefs.pm (working copy)
> @@ -625,20 +625,28 @@
> my $value = shift;
> my $ind = shift;
>
> + my $oldValue = '';
> +
> if (defined $ind) {
>
> if (defined $prefs{$key}) {
> +
> if (ref $prefs{$key} eq 'ARRAY') {
> +
> if (defined($prefs{$key}[$ind]) && defined($value) && $value eq $prefs{$key}[$ind]) {
> - return;
> + return;
> }
>
> + $oldValue = $prefs{$key}[$ind];
> $prefs{$key}[$ind] = $value;
> +
> } elsif (ref $prefs{$key} eq 'HASH') {
> +
> if (defined($prefs{$key}{$ind}) && defined($value) && $value eq $prefs{$key}{$ind}) {
> - return;
> + return;
> }
>
> + $oldValue = $prefs{$key}{$ind};
> $prefs{$key}{$ind} = $value;
> }
> }
> @@ -652,12 +660,18 @@
> } else {
>
> if (defined($prefs{$key}) && defined($value) && $value eq $prefs{$key}) {
> - return;
> + return;
> }
>
> + $oldValue = $prefs{$key};
> $prefs{$key} = $value;
> }
>
> + # Don't bother writing out if nothing has changed.
> + if (defined $value && defined $oldValue && $value eq $oldValue) {
> + return;
> + }
> +
> onChange($key, $value, $ind);
>
> # must mark $ind as defined or indexed prefs cause an error in this msg

Dan Sully
2005-07-21, 16:25
* Robert Moser shaped the electrons to say...

>It's never going to hit the case where $oldValue eq $value, since that
>possibility is precluded by all the checks made prior to setting the
>preference (see all those returns you shifted left).
>
>Hopefully the r3770 change I just put in will help. It writes out the
>prefs file in the cleanup code if there is a write pending.

Ok.. I'd still like to get rid of the scheduleWrite code if at all possible.

-D
--
<dr.pox> wtf? a garbled dingbat makes java switch to DWIM?

Grotus
2005-07-21, 16:44
Dan Sully wrote:
> * Robert Moser shaped the electrons to say...
>
>> It's never going to hit the case where $oldValue eq $value, since that
>> possibility is precluded by all the checks made prior to setting the
>> preference (see all those returns you shifted left).
>>
>> Hopefully the r3770 change I just put in will help. It writes out the
>> prefs file in the cleanup code if there is a write pending.
>
>
> Ok.. I'd still like to get rid of the scheduleWrite code if at all
> possible.
> -D

The scheduleWrite is there mainly just to prevent writing out the prefs
file repeatedly when multiple changes come all at once. The default 30
seconds is probably too long. Changing that to something like 2 to 10
seconds would most likely be sufficient to keep us from writing too much
while at the same time significantly reducing the window for people to
make a change, then kill the server.

Dan Sully
2005-07-21, 16:44
* Robert Moser shaped the electrons to say...

>The scheduleWrite is there mainly just to prevent writing out the prefs
>file repeatedly when multiple changes come all at once. The default 30
>seconds is probably too long. Changing that to something like 2 to 10
>seconds would most likely be sufficient to keep us from writing too much
>while at the same time significantly reducing the window for people to
>make a change, then kill the server.

Yeah - let's move it to the 5-10 second range.

-D
--
( ( ( [ ] ) ) )
In Stereo Where
Available

kdf
2005-07-21, 16:52
Quoting Dan Sully <dan (AT) slimdevices (DOT) com>:

> * Robert Moser shaped the electrons to say...
>
> >The scheduleWrite is there mainly just to prevent writing out the prefs
> >file repeatedly when multiple changes come all at once. The default 30
> >seconds is probably too long. Changing that to something like 2 to 10
> >seconds would most likely be sufficient to keep us from writing too much
> >while at the same time significantly reducing the window for people to
> >make a change, then kill the server.
>
> Yeah - let's move it to the 5-10 second range.

This being the case, it might also be wise to consider methods for forcing the
change on users who already have the pref set at 30 from running into potential
problems in the future.

I set mine to zero on the same day as the change was made, personally. However,
if the SIGINT write works, I'll have no problem with turning on the delay
again. Delays were not friendly to testing :)

-kdf

Grotus
2005-07-21, 19:46
kdf blurted out:
>
>
> This being the case, it might also be wise to consider methods for forcing the
> change on users who already have the pref set at 30 from running into potential
> problems in the future.

Update script would be the way to do it. If it currently is at 30, set
it to the new default.

> I set mine to zero on the same day as the change was made, personally. However,
> if the SIGINT write works, I'll have no problem with turning on the delay
> again. Delays were not friendly to testing :)

I set mine to 0 too. For developers who stop and start the server
constantly, it definitely was a pain. I'm thinking that the write on
cleanup will cure that though.

Dan Sully
2005-07-21, 23:12
Thoughts on making each client have a real array of prefs?

Instad of the $mac-$prefName that's currently there.

-D
--
<dr.pox> wtf? a garbled dingbat makes java switch to DWIM?

Triode
2005-07-22, 00:41
> Yeah - let's move it to the 5-10 second range.
>

I think it would be worthwhile keeping some slight delay between a user changing something+relavent code being involked and writing
out prefs - only to avoid the case of a certain preference value causing repeated server crashes.

I'm sure everyone elses code is much better than mine though...

Adrian

kdf
2005-07-22, 01:32
Quoting Dan Sully <dan (AT) slimdevices (DOT) com>:

> Thoughts on making each client have a real array of prefs?
>
> Instad of the $mac-$prefName that's currently there.

in yaml, I assume this looks something like:
playerMAC :
- pref1
- pref2

etc.

I think I like that. Its a bit much having a pref file flooded with hex.

could the defaults be grabbed from there too, say if you edited to have a
section headed by "defaultplayer" ? I believe there is a feature requeast for
default player settings. it would aid in deployment situations, I guess.

btw, the field Base patch is working ok so far. kinda ditched a patch I was
working on, but it should be easy to move it over. It all depends on whether
having artwork shown in any album context is worth having or not. it would
make browse artwork unnecessary, and less confusing (well, that's my hope).

-kdf

Grotus
2005-07-22, 12:03
Dan Sully blurted out:
> Thoughts on making each client have a real array of prefs?
>
> Instad of the $mac-$prefName that's currently there.
>
> -D

Yep, I was going to do that after the YAML introduction, since it makes
it much easier.

Grotus
2005-07-22, 12:23
Robert Moser blurted out:
> Dan Sully blurted out:
>
>> Thoughts on making each client have a real array of prefs?
>>
>> Instad of the $mac-$prefName that's currently there.
> Yep, I was going to do that after the YAML introduction, since it makes
> it much easier.

To be a bit more specific, I was going to get rid of all the clientX
functions (except maybe as wrappers for $client->X) and have the client
object handle the client preferences.

To do this, there would be a function in Prefs for the client to get a
reference to its preference hash. The client object would be
responsible for the onChange stuff. When something changes it would do
a Slim::Utils::Prefs::scheduleWrite unless
Slim::Utils::Prefs::writePending().

But that will have to wait until I get back from Yosemite.

Grotus
2005-07-22, 12:57
kdf blurted out:
> Quoting Dan Sully <dan (AT) slimdevices (DOT) com>:
> in yaml, I assume this looks something like:
> playerMAC :
> - pref1
> - pref2
>
> etc.
>
> I think I like that. Its a bit much having a pref file flooded with hex.

Not quite what I had in mind. What I would like would be more like this
(probably not actual YAML here, but it would be in the file):

CLIENTPREFS:
playerMAC:
pref1: value1
pref2: value2

In other words, have all the client pref hashs be under a single key in
the main pref hash. That way you can get a list of known client ids
with Slim::Utils::Prefs::getKeys('CLIENTPREFS') and the actual client
pref hash with Slim::Utils::Prefs::getInd('CLIENTPREFS',$client->id());


> could the defaults be grabbed from there too, say if you edited to have a
> section headed by "defaultplayer" ? I believe there is a feature requeast for
> default player settings. it would aid in deployment situations, I guess.

We could add a layer for default setting, but we would still want to
have some defaults set in the main code, just in case the pref file was
totally removed. So, add a DEFAULTCLIENT under CLIENTPREFS, and in the
pref verification code first check to see if
$prefs{CLIENTPREFS}{DEFAULTCLIENT}{$pref} is defined, then look to
$defaults{$pref}.

cdoherty
2005-07-23, 08:36
On Fri, Jul 22, 2005 at 08:41:07AM +0100, Triode said:
> I think it would be worthwhile keeping some slight delay between a user
> changing something+relavent code being involked and writing out prefs -
> only to avoid the case of a certain preference value causing repeated
> server crashes.
>
> I'm sure everyone elses code is much better than mine though...

isn't that an edge case only useful for developers? from a user point of
view, having a pref value crash the server now or 30 seconds from now
isn't a huge difference. :-)

what's the benefit to delaying pref writes at all? I don't notice any
slowdown from writing prefs out every time--do others notice a performance
hit?

chris


-------------------------------
Chris Doherty
chris [at] randomcamel.net

"I think," said Christopher Robin, "that we ought to eat
all our provisions now, so we won't have so much to carry."
-- A. A. Milne
-------------------------------

Triode
2005-07-23, 09:44
> isn't that an edge case only useful for developers? from a user point of
> view, having a pref value crash the server now or 30 seconds from now
> isn't a huge difference. :-)
>

You won't like it if every time the server starts it crashes and the only way to get out of it is to manually edit the preference
file.. I would suggest it is a useful safety measure.

Of course we could mandate all code is bug free...

Philip Meyer
2005-07-25, 00:27
>Here's a quick patch that will only write out prefs if a value has changed.
>
>I'd like to use this to get rid of the scheduledWrite code, which has been
>causing support isses.
>
Has the preference writing code been changed recently? I had a problem a couple of days ago whereby I lost all my preferences. I think it may have been the first time I ran 6.2. I haven't reported the bug, as I have upgraded to the latest nightly and haven't seen an issue since.

Perhaps it would be a good idea to automatically store a backup; perhaps copy the prefs off to a backup once a week, or each time the server is restarted (whichever occurs sooner).

Phil

Philip Meyer
2005-07-25, 00:50
>You won't like it if every time the server starts it crashes and the only way to get out of it is to manually edit the preference
>file.. I would suggest it is a useful safety measure.

If the default write is every 10 secs, what's the difference? A dodgy preference setting could still get into the pref file, and cause the server to crash at startup. I see no point having a delay.

Better would be to have a backup file, perhaps with some kind of mechanism that would use the backup if the previous invokation of slimserver didn't exit tidily. Eg. when catching a fatal error, as well as logging the error, replace the previous preference backup (or touch an indicator file, which would cause slimserver to read the previous pref file when it is restarted).

In all my time of using slimserver (over a year now) and seeing it crash, I can only remember one time where this has been directly due to changing a preference value (and that was a long time ago), so Slimserver reliability has perhaps improved. That was when I changed the date format to British and it killed the server. Best way forward is to protect from crashes, or stop the crashes in the first place.

Phil

kdf
2005-07-25, 01:07
Quoting Philip Meyer <slim (AT) hergest (DOT) demon.co.uk>:

> >You won't like it if every time the server starts it crashes and the only
> way to get out of it is to manually edit the preference
> >file.. I would suggest it is a useful safety measure.
>
> If the default write is every 10 secs, what's the difference? A dodgy
> preference setting could still get into the pref file, and cause the server
> to crash at startup. I see no point having a delay.

while the server is running, it grabs te prefs from memory. so, if setting a
pref with a bad value causes a crash, then it wont be written permanently. Not
a huge concern for most, certain not worth a big debate.

> Best way forward is to protect from crashes, or stop
> the crashes in the first place.

This is always the intent.

-kdf

Philip Meyer
2005-07-25, 11:44
>while the server is running, it grabs te prefs from memory. so, if setting a
>pref with a bad value causes a crash, then it wont be written permanently. Not
>a huge concern for most, certain not worth a big debate.
>
But a pref change that will only cause a crash when the server is restarted will still immediately crash the server when it is restarted whether the pref is written immediately or 30 secs after changing! Eg, enabling a plugin that requires a server restart.

Also, if a change to prefs causes a crash, it might be easier to track down by the user community if it happens instantly.

Providing there was a backup of the previous prefs that is easy to go back to, I think it would be worth changing.

>> Best way forward is to protect from crashes, or stop
>> the crashes in the first place.
>
>This is always the intent.
>
Just checking ;)

Phil