spotty.prefs unnecessary writes due to $prefs->set('displayNames') prevents disk spin-down (or, causes regular disk spin-up)

[stripwax]

Relevant for people like me who have aggressive hard drive spindown settings. Maybe avoids unnecessary wear for the SSD crew too.. Looks like an easy fix. currently, $prefs->set happens (which causes disk to spin up) in AccountHelper.pm setName function, but it seems like this happens even when $names isn't changing. As a result, even though nothing actually changes, the spotty.prefs gets touched on-disk anyway. The actual change that gets written to spotty.prefs is just the _ts_displayNames (confirmed via diffing), confirming the spinup and write was unnecessary.

In my case I was seeing pretty regular disk spinup (5-10 mins) attributed to this.

Trying out a local change, I can provide pull request if it works.

[michaelherger]

There must be more to this, as the underlying prefs saving code already does prevent writing out un-changed values:

https://github.com/Logitech/slimserver/blob/b821d9437ddc27f01f0c5b33d4346c6d9da516ad/Slim/Utils/Prefs/Base.pm#L96

Please let me know if you find something. You could enable debug logging for prefs (Settings/Advanced/Logging) to see what's going on.

I'll be happy to fix this.

[stripwax]

Could it be because displayNames is a list, not a scalar? That suppress code has a comment saying its only for scalars. (Maybe that means the fix should be to the set function, I hadn't thought of that. I can't think of a reason why it would need to write non-scalars when they haven't changed. Presumably for simplicity in comparing scalars.)

[michaelherger]

You're up to something there. While it's not a list, it's a hash. Would the following change work for you?

diff --git a/AccountHelper.pm b/AccountHelper.pm
index 9565879..354ff97 100644
--- a/AccountHelper.pm
+++ b/AccountHelper.pm
@@ -378,8 +378,14 @@ sub setName {
 
        if ($result && $result->{display_name}) {
                my $names = $prefs->get('displayNames');
-               $names->{$userId} = $result->{display_name};
-               $prefs->set('displayNames', $names);
+
+               my $oldName = $names->{$userId};
+               my $newName = $result->{display_name};
+
+               if ((!$oldName && $newName) || ($newName && $oldName ne $newName)) {
+                       $names->{$userId} = $result->{display_name};
+                       $prefs->set('displayNames', $names);
+               }
        }
 }
 

[stripwax]

I'll try it. I was actually just trying this instead, to see what happens:

if ($names->{userId} != $result->{display_name}) { ...

On Fri, 5 Aug 2022, 07:51 Michael Herger, @.***> wrote:

You're up to something there. While it's not a list, it's a hash. Would the following change work for you?

diff --git a/AccountHelper.pm b/AccountHelper.pm index 9565879..354ff97 100644--- a/AccountHelper.pm+++ b/AccountHelper.pm@@ -378,8 +378,14 @@ sub setName {

    if ($result && $result->{display_name}) {
            my $names = $prefs->get('displayNames');-               $names->{$userId} = $result->{display_name};-               $prefs->set('displayNames', $names);++               my $oldName = $names->{$userId};+               my $newName = $result->{display_name};++               if ((!$oldName && $newName) || ($newName && $oldName ne $newName)) {+                       $names->{$userId} = $result->{display_name};+                       $prefs->set('displayNames', $names);+               }
    }

}

— Reply to this email directly, view it on GitHub https://github.com/michaelherger/Spotty-Plugin/issues/70#issuecomment-1206106096, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABF47HHPG3CFQ3UHF6LSWC3VXS2WBANCNFSM55UBLYRQ . You are receiving this because you authored the thread.Message ID: @.***>

[michaelherger]

if ($names->{userId} != $result->{display_name}) { ...

That's basically the same. My version has some sanity checks in addition - and it uses the correct operator for string comparisons (!= vs. ne - see eg. https://users.cs.cf.ac.uk/dave/PERL/node37.html).

[michaelherger]

Did that work?

[stripwax]

Apologies, I haven't tried it yet. Your proposed fix seems overcomplicated though - is testing $newName really necessary? The outer IF has already asserted that result->{'display_name'} is valid - doing the check again twice in the inner IF didn't make sense to me.

[stripwax]

Yep your change appears to work

[michaelherger]

Your proposed fix seems overcomplicated though - is testing $newName really necessary? The outer IF has already asserted that result->{'display_name'} is valid - doing the check again twice in the inner IF didn't make sense to me.

Good catch, thanks!