Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
51 stars 10 forks source link

System slowdown with some Apps #7

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

As reported in a recent comment, some people appear to be experiencing system freezes when the extension is active which go away when turned off:

I wonder if other similar has the same issue. If not, it would be worth checking why not and if they have implemented any workaround.

It would also be good to know if previous versions of the extension experience the same thing and how much increasing the refresh-rate improve things.

Ultimately, updating text/icon only upon change would be preferred.

Moon-0xff commented 1 year ago

v3 (bffdcd46f4ff7bb286deb8970df80b0e94532570) + Shortwave doesn't freezes the shell. Or at least it isn't too noticeable.

Moon-0xff commented 1 year ago

The D-Bus handling code of mpris-indicator-button includes some error checking among other things. We should check if Shortwave works fine with this extension.

Moon-0xff commented 1 year ago

Yes, mpris-indicator-button works fine with Shortwave.

Moon-0xff commented 1 year ago

Based on the latest comment on ego (not mine obviously) it looks like the shell freezes for the amount of time D-Bus gives until timeout with certain applications.

Moon-0xff commented 1 year ago

All the applications mentioned are somewhat related to GNOME. can it be a problem with glib, gtk or another GNOME toolkit?

Batwam commented 1 year ago

Yeah, it's pretty odd. I just tried Videos/Totem and I can't seem to experience anything. Shortwave is somewhat noticeable with some stutter every time it refreshes.

I think that the best thing to do will be to figure out which specific calls are causing the slowdowns to avoid focusing on the wrong parts.

I have a couple of ideas which might help reduce the number of dbus calls but for initial troubleshooting it would be good to monitor response times over the various dbus calls to understand what is hanging.

Is anyone getting freezes during first source app launch and then it works fine?

Batwam commented 1 year ago

ok, I think that we should tackle this issue next. I have some ideas to reduce some of the calls without affecting the way the extension currently works but I'll let close you the install instructions first to avoid having multiple pull requests open in parrallel.

Batwam commented 1 year ago

ok, I've tried to check the time taken for the various DBus calls throughout the code in the "speed-test" branch and I think that the results speak for themselves. This app takes ~50 times longer than the others to respond! image

As a minumim, we might want to increase the refresh rate if a source has such slow response times (perhaps even make the refresh-rate somewhat proportional to the refresh-rate if we want to be fancy...). We could also disable options which required to know the playbackStatus. This would mean only one call to the app per cycle.

It also shows that we are probably making Status calls we don't need to make (we are checking the status of players which aren't the active one). I had a quick look at this. I initially removed the playback when the option to hide label when paused is removed but later realised that the status is used in order parts of the code, including AutoSwitch (to build the activePlayersList so it's not straightforward but we should be able to reduce the number of calls we do if we put our mind to it.

We should also only request the Metadata string from DBUs only once per cycle and filter based on the fields required rather than requesting it from the source for each field (potentially 3 times per cycle). Is this something you could implement please?

I also tweaked the metadata code to check if the field is blank first and exit before seeking the metadata as the original call would always ask DBus for the metadata string, then check, then exit if blank.

Moon-0xff commented 1 year ago

Good detective work, but i don't think any of those changes will solve the problem.

Don't worry about having multiple pull requests active, specially because they don't collide with each other in any way.

I will be making changes to the D-Bus related code and pushing them to the dbus-changes branch, you might want to sync your branch to this one.

Batwam commented 1 year ago

Yeah, ultimately the response time is too long so it hangs. If we could take that down to the same level as the others the lack of optimisation would be less noticeable or drastically reduce the number of requests by making it asynchronous.

What other solution do you have in mind? Have you found a way to get dbus to notify the extension of the changes?

The fork is mostly for testing purposes with a bunch of logs and not so much code change so I'll wait to see how you progress on these dbus changes.

Moon-0xff commented 1 year ago

Correct me if I'm wrong, but i don't think this concept of requests apply to async threads.

MPRIS notifies property changes through a signal

Moon-0xff commented 1 year ago

Basically. Check the async programming guide.

Batwam commented 1 year ago

yeah, I'm not sure to be honest, it's well beyond my level of expertise. I'd need to see it implemented to understand. Have you seen any extension using this?

To me, it looks similar to interrupts in C but at the end of the day, we would still need to find out when the active source has changed and extract the metadata from the sources so, unless I'm missing something, the bulk of the current code (and associated slow DBUS calls) should still apply but hopefully much more rarely (only when a change is detected rather them several times per second...)? or you are hoping to be running these mpris data requests in parrallel to the main loop and that would avoid slowdowns?

Moon-0xff commented 1 year ago

Javascript is a single-threaded language. The shell is written in javascript. Extensions are part of the shell once enabled. So if our extension stops, the entire shell stops.

using a different process thread to handle events will avoid this. The same way as when a program freezes, it doesn't freezes the entire computer (most of the times).

That's how i understand it at least. I'm not used to async programming.

Batwam commented 1 year ago

Sounds like a different version of the extension altogether. Not sure how quickly that can be put together. There is a thread about async extension here, perhaps some example code to use as a starting point: https://stackoverflow.com/questions/67411285/asynchronous-extension-breaks-gnome-shells

Edit: arg, the repo is gone and the link is the same as the one above!

In the meantime, I might still push some simple optimisations to the current implementation to improve the workflow if that's ok.

Moon-0xff commented 1 year ago

In the meantime, I might still push some simple optimisations to the current implementation to improve the workflow if that's ok.

Sure, go ahead!

Moon-0xff commented 1 year ago

Sounds like a different version of the extension altogether. Not sure how quickly that can be put together.

Yes, I'm starting from scratch (for testing purposes). I don't know how much time this will take but probably several weeks. Perhaps I'm too optimistic pessimistic.

Batwam commented 1 year ago

Issue reported on the shortwave side: https://gitlab.gnome.org/World/Shortwave/-/issues/684

Hopefully we can minimise the effect on our side while they fix it on the shortwave side. I still don't understand why it's taking shortwave so much longer so I really hope that they can look into it and get to the bottom of it all. Hopefully being a gnome app, they will be more receptive than if it was something like spotify or chrome.

and yes, there might be other apps which would require fixing to but shortwave seems to be worked on quite actively.

Moon-0xff commented 1 year ago

I'm trying to build a tiny version of this extension capable of dealing with the slow responses of shortwave under the minimal branch.

I don't have much experience doing asynchronous programming, and I've never done it in javascript, so this is more of a learning process than a development process.

Batwam commented 1 year ago

Hey, I checked out the minimal branch. It appears to be working (as long as I create a schemas folder for gsettings). The code doesn't look too different, is the plan to keep it minimal, or update the base code to match the async method (or maybe even both)?

One thing I noticed though is that if I run my highly scientific test of pressing a key and see if it hangs, I notice that it still hangs every second with shortwave. Obviously it's not as bad as the old extension which was running 4-5 dbus calls per cycle but it's still there. Did you notice that too?

Moon-0xff commented 1 year ago

Hello @Batwam, hope you are feeling well. I noticed more than that, it looks like almost all the delay comes from redefining the player proxy. Check out the structure-changes branch and enable the time logs.

Batwam commented 1 year ago

Yeah, feeling a but better now, thanks!

I just checked out the other branch, very impressive indeed!

Batwam commented 1 year ago

I noticed that the icon padding on the right is no longer adjusted to remove the excess 5 pts. when looking in more detail, I realised that the mechanism in _onPaddingChanged(){ within if(SHOW_ICON) is ignored as we have the line const SHOW_ICON = this.settings.get_int('show-icon'); as int while show-icon is a boolean.

However, if I swap for const SHOW_ICON = this.settings.get_boolean('show-icon');, the extension no longer loads which is odd as it works just fine within _setIcon()...

Moon-0xff commented 1 year ago

Because constants can't be redefined. I pushed a fix just now, good catch.

Moon-0xff commented 1 year ago

So, as far as i know/tested #13 solves short-freezes, but what about long freezes? I know they are a thing because i have experienced them with rhythmbox before, it never occurred to me it was a problem with the extension though.

Batwam commented 1 year ago

Same here, I've experienced it exclusively with Rhythmbox but it hasn't occurred for a while. Somehow, I have the feeling that the latest changes might help this issue as a by-product.

If the freeze is due to an absence of response and dbus ~25s timeout, I wonder if there is a way to get the extension to cancel a request if it takes more than say 0.5s (or even 1s to be safe) for the response to come through?

Moon-0xff commented 1 year ago

Yes we can. But judging by how mpris.js handles things, i don't think it's necessary at all.

Moon-0xff commented 1 year ago

My theory is all the D-Bus stuff is handled by the proxy without slowing down or bothering the main thread except when it's declared. If that's the case then simply adding a function callback to the proxy declaration should solve this issue.

edit: i skipped some info here, according to gjs.guide, the proxy will be created asynchronously if a callback is passed

Moon-0xff commented 1 year ago

Well, i just did it. It doesn't seem to create any problem, but does it solve the issue?

Batwam commented 1 year ago

Only time will tell I guess. I just checked with Videos/Totem and Rhythmbox and had no issue.

In the meantime, the latest changes vastly improved the cycle times so I'd suggest releasing this in gnome extensions when you are comfortable as it's probably going to help a few people currently experiencing issues. You could also include an extra screenshot to show the icon?

Moon-0xff commented 1 year ago

There's a few things to do before release. Mostly testing. I will submit a "roadmap" for v5 soon enough. edit: roadmap is here #14

Moon-0xff commented 1 year ago

Shortwave generates errors for GNOME mpris.js too

JS ERROR: Exception in callback for signal: hide: Error: Impossible to remove untracked message
                                        removeMessage@resource:///org/gnome/shell/ui/messageList.js:680:19
                                        _addPlayer/<@resource:///org/gnome/shell/ui/mpris.js:271:18
                                        _emit@resource:///org/gnome/gjs/modules/core/_signals.js:114:47
                                        _updateState@resource:///org/gnome/shell/ui/mpris.js:234:22
                                        _onPlayerProxyReady/<@resource:///org/gnome/shell/ui/mpris.js:183:48
Moon-0xff commented 1 year ago

This latest version works well for the user that initially raised the issue. Amazing! Time to close this.