Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Further reduce CPU usage when refreshing UI #171

Merged
merged 4 commits into from
Oct 22, 2023
Merged

Further reduce CPU usage when refreshing UI #171

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
561b9d7
Further reduce CPU usage when refreshing UI
skyline75489 Oct 16, 2023
872fdcd
Occlusion state, baby
skyline75489 Oct 19, 2023
aeb4f50
Also handle it in updateProgress
skyline75489 Oct 19, 2023
ffe2eca
Rm observers on dealloc
skyline75489 Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions Submariner/SBDatabaseController.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,13 @@ - (void)windowDidLoad {
name:@"SBSubsonicConnectionFailedNotification"
object:nil];

// observe window state
[[NSNotificationCenter defaultCenter] addObserver:self
NattyNarwhal marked this conversation as resolved.
Show resolved Hide resolved
selector:@selector(windowDidChangeOcclusionState:)
name:NSWindowDidChangeOcclusionStateNotification
object:nil];


// setup main box subviews animation
// XXX: Creates a null first item
SBNavigationItem *navItem = [[SBLocalMusicNavigationItem alloc] init];
Expand Down Expand Up @@ -965,14 +972,21 @@ - (void)uninstallProgressTimer {
}
[progressUpdateTimer invalidate];
progressUpdateTimer = nil;
[self clearPlaybackProgress];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure it's a good idea to remove this? Whenever playback ends, it's usually uninstallProgressTimer calling clearPlaybackProgress, not updateProgress (though the timer might trigger first). If anything, it might be good to remove it from updateProgress since uninstallProgressTimer should take care of it (though it won't hurt).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention is to make uninstallProgressTimer more focused on, well uninstalling the progress timer and does nothing else. So I just moved this line somewhere else.

}

- (void)updateProgress:(NSTimer *)updatedTimer {

if([[SBPlayer sharedInstance] isPlaying]) {
[progressSlider setEnabled:YES];
if ([[SBPlayer sharedInstance] isPaused]) {
return;
}

BOOL visible = self.window.occlusionState & NSWindowOcclusionStateVisible;
if (!visible) {
return;
}

[progressSlider setEnabled:YES];
NSString *currentTimeString = [[SBPlayer sharedInstance] currentTimeString];
NSString *remainingTimeString = [[SBPlayer sharedInstance] remainingTimeString];
double progress = [[SBPlayer sharedInstance] progress];
Expand Down Expand Up @@ -1364,6 +1378,27 @@ - (void)subsonicConnectionSucceeded:(NSNotification *)notification {



#pragma mark -
#pragma mark Window Notification (Private)

- (void)windowDidChangeOcclusionState:(NSNotification *)notification {
NSWindow *sender = [notification object];
if ([sender isEqual:self.window]) {
BOOL visible = self.window.occlusionState & NSWindowOcclusionStateVisible;
if (visible) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would install the timer even if it's stopped/paused when it becomes visible (et vice versa)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I see that updateProgress will try to return early to avoid doing work. That does still make the timer wake up though, but if you're already playing, I guess it doesn't matter too much

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the return early part is to prevent high CPU usage under this scenario: play a song->minimize the window->current song finished, next song started while minimized (update timer will still be installed and start running)

Copy link
Author

@skyline75489 skyline75489 Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think about it, maybe we should only do the early return here and avoid all the timer juggling? Early return reduces the CPU usage basically as much as uninstalling the timer, at least from my testing. Though in the pursuit of perfection, no timer should be running, that I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could check for playback before (un)installing the timer, but another way to do is is centralize the checks into early returns into those methods, so the caller doesn't need to be aware of the taste.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't like the central check here inside updateProgress? I think it kinda beats early checks in all the playback-related methods. Under the previous scenario I mentioned, only the check inside updateProgress prevents the unnecessary UI refreshing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but it might be better to put those checks in the timer installation (or both) and avoid having the timer tick in the first place. I'm fine either way, but it might be worth experimenting to see the impact of a timer that returns early versus uninstalling it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, I see no obvious difference between early return in updateProgress and uninstalling the timer. The CPU usage are both quite low, at around 1%-2%.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case it's probably fine then.

[self installProgressTimer];
}
else {
[self uninstallProgressTimer];
}
}
}






#pragma mark -
#pragma mark Player Notifications (Private)

Expand Down Expand Up @@ -1406,6 +1441,7 @@ - (void)playerPlayStateNotification:(NSNotification *)notification {
}
} else {
[self uninstallProgressTimer];
[self clearPlaybackProgress];
NattyNarwhal marked this conversation as resolved.
Show resolved Hide resolved
[playPauseButton setState:NSControlStateValueOn];
}
}
Expand Down
4 changes: 2 additions & 2 deletions Submariner/SBPlayer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ extension NSNotification.Name {
}

@objc var currentTimeString: String {
return String(time: currentTime)
return String(timeInterval: currentTime)
}

@objc var durationTime: TimeInterval {
Expand All @@ -782,7 +782,7 @@ extension NSNotification.Name {
}

@objc var remainingTimeString: String {
return String(time: remainingTime)
return String(timeInterval: remainingTime)
}

@objc var progress: Double {
Expand Down
2 changes: 1 addition & 1 deletion Submariner/SBTrack.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class SBTrack: SBMusicItem {

@objc var durationString: String? {
self.willAccessValue(forKey: "duration")
let ret = String(time: TimeInterval(duration?.intValue ?? 0))
let ret = String(timeInterval: TimeInterval(duration?.intValue ?? 0))
self.didAccessValue(forKey: "duration")
return ret
}
Expand Down
18 changes: 14 additions & 4 deletions Submariner/String+Time.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,22 @@ extension String {
return String.rfc3339DateFormatter.date(from: self as String)
}

init(time: TimeInterval) {
if time == 0 || time.isNaN {
self = "0:00"
NattyNarwhal marked this conversation as resolved.
Show resolved Hide resolved
init(timeInterval: TimeInterval) {
if timeInterval == 0 || timeInterval.isNaN {
self = "00:00"
return
}

let ti = Int(timeInterval)
let seconds = ti % 60
let minutes = (ti / 60) % 60
let hours = (ti / 3600)

self = String.componentFormatter.string(from: time)!
if (hours > 0) {
self = String(format: "%0.2d:%0.2d:%0.2d", hours, minutes, seconds);
}
else {
self = String(format: "%0.2d:%0.2d", minutes, seconds);
}
}
}