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.

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

Performance Issues and discussion #39

Closed
4 tasks
dgcampea opened this issue Jul 20, 2021 · 20 comments
Closed
4 tasks

Performance Issues and discussion #39

dgcampea opened this issue Jul 20, 2021 · 20 comments
Assignees

Comments

@dgcampea
Copy link
Contributor

dgcampea commented Jul 20, 2021

  • 1. all artists is unusable with very large library collections, the indexing is too slow.
    Caching results and cover images could result in performance boosts. Having it work asynchronously can improve responsiveness too.

    • 1.1 Even showing albums from an artist is somewhat unresponsive since it blocks while retrieving. Not sure if the bulk of the responsiveness issue is due to cover image retrieval but they could be retrieved in the background and displayed as they come (showing the "default" no cover image while waiting).
    • 1.2. GdkPixbuf.Pixbuf.new_from_file_at_size() gets performed multiple times for cover-less albums/tracks when it can be cached/done once at application startup. (it's using the default CD image after all)
  • 2. mpd update handling: currently done using mpd protocol command status. A better way would be to use idle command (python-mpd2 has MPDClient.idle) that can inform when a database update was finished which can then trigger a mpdevil update instead of triggering multiple updates while mpd hasn't even finished (every "mpdevil update" resets the library navigation position). This might not be too bad if mpd is updated manually but under auto updates from mpd it's infuriating to have the UI jump around at times or even unusable if the library changes are large.

Others:

  • Can shuffle all tracks or albums be efficiently done with mpdevil? Solving 1. does not necessarily imply this.

I can offer to look into 1.2 and 2 but I doubt I can be of any help about the rest of the points.

@SoongNoonien
Copy link
Owner

SoongNoonien commented Jul 22, 2021

Not sure if the bulk of the responsiveness issue is due to cover image retrieval but they could be retrieved in the background and displayed as they come (showing the "default" no cover image while waiting).

These are the times on my machine when showing all artists:
get artists 0.005196809768676758
get albums 3.4528298377990723
render covers 5.29137110710144
display covers 0.19960594177246094

"get albums" also includes fetching the covers. When loading covers directly with MPD this changes to:
get artists 0.005057096481323242
get albums 5.746363878250122
render covers 7.412358283996582
display covers 0.20219874382019043

So, it's not the bulk but still a great factor. "render covers" is the conversation from binary data or a file path to a GdkPixbuf, but this is not blocking the UI. In fact only "get artists" is really blocking the UI, "get albums" can block the UI when an artist with many albums gets processed, this may also be the issue 1.1.

GdkPixbuf.Pixbuf.new_from_file_at_size() gets performed multiple times for cover-less albums/tracks when it can be cached/done once at application startup. (it's using the default CD image after all)

Yes, that's right but it doesn't take much time. Performing this once when "AlbumWindow._refresh" is executed would be fine but at application startup is not possible since the cover size might change.

mpd update handling: currently done using mpd protocol command status. A better way would be to use idle command (python-mpd2 has MPDClient.idle) that can inform when a database update was finished which can then trigger a mpdevil update instead of triggering multiple updates while mpd hasn't even finished (every "mpdevil update" resets the library navigation position). This might not be too bad if mpd is updated manually but under auto updates from mpd it's infuriating to have the UI jump around at times or even unusable if the library changes are large.

I'd really like to use idle but this completely blocks the UI. Also it is not possible to run any other command when the client is idling. To get the current bitrate it would still be necessary to fetch the status periodically (the seek bar could be updated differently). An old version of mpdevil used idle in a very weird way. To be able to use the UI the idling was periodically interrupted and a second instance of MPDClient was used to handle all other commands. This was way to complex so I replaced it by a simpler solution.
I'm not aware of triggering "update" multiple times. "update" gets emitted exactly once when the "updating_db" key vanishes from status dict and this key should only vanish when the job is done. In fact I changed this a long time ago to fix the issue you are describing. Since then the issue was fixed on all my machines.

@dgcampea
Copy link
Contributor Author

These are the times on my machine when showing all artists:
get artists 0.005196809768676758
get albums 3.4528298377990723
render covers 5.29137110710144
display covers 0.19960594177246094

What are you using to measure these timings?

An old version of mpdevil used idle in a very weird way.

Can you point from which commit was idle being used until you removed it?

@SoongNoonien
Copy link
Owner

What are you using to measure these timings?

I'm using the time module of python. I patched mpdevil accordingly just for this test.

Can you point from which commit was idle being used until you removed it?

Version 0.6.0 was the first version with idle and 0.8.3 the last. Commit 7391686 introduced it and commit db7a8e9 removed it.

@SoongNoonien
Copy link
Owner

According point 1.:

Caching results and cover images could result in performance boosts.

Yes, that's right but I like to avoid a client side cache or database.

Having it work asynchronously can improve responsiveness too.

I'm open for suggestions. But I think this is not easy I've spend much time on this and it's already faster than it was.

@dgcampea
Copy link
Contributor Author

dgcampea commented Jul 22, 2021

I'm using the time module of python. I patched mpdevil accordingly just for this test.

You can test with cProfile without changing the code. I'm using it as python -m cProfile -o report /app/bin/mpdevil with snakeviz to see the report.
I can confirm that get_albums() is the bulk of the issue here.
As a sidenote, I get almost 30+ secs of cumulative time for get_cover_binary.

@SoongNoonien
Copy link
Owner

How long does the whole process of showing all artists take on your setup?

As a sidenote, I get almost 30+ secs of cumulative time for get_cover_binary.

Ok, but this is limited by MPD the function just fetches the covers with a standard MPD command. Any suggestions on speeding this up?

@dgcampea
Copy link
Contributor Author

How long does the whole process of showing all artists take on your setup?

7m42s ~ 8m51s

@SoongNoonien
Copy link
Owner

Ok, that's way to long. Can you send me a list similar to mine so I can see how long each step takes?

@dgcampea
Copy link
Contributor Author

Will a python cProfile output do?

@SoongNoonien
Copy link
Owner

I never used them before but I think it'll to. Btw another question: You said your GUI is blocking, when and how long?

@dgcampea
Copy link
Contributor Author

Btw another question: You said your GUI is blocking, when and how long?

My bad, I just went back and checked it, it doesn't actually block but it becomes very "sluggish" and can give the impression that it's blocked.

@SoongNoonien
Copy link
Owner

I think it should be possible to reduce the time needed for get_albums by using "tagtypes" and "count". I'll look into it after the next release.

@SoongNoonien
Copy link
Owner

968158d uses tagtypes and count to reduce data traffic and the album covers get now rendered while the albums are already visible. Please let me know if this reduces the loading duration on your setup.

@dgcampea
Copy link
Contributor Author

968158d uses tagtypes and count to reduce data traffic and the album covers get now rendered while the albums are already visible. Please let me know if this reduces the loading duration on your setup.

Not much of an improvement seen here, it's still taking several minutes.

@SoongNoonien
Copy link
Owner

Ok, but MPD traffic is now at a minimum. I currently don't see any room for improvements. May I ask how large your collect is? Mine is about 5000 songs and 400 albums. Just to see if the time grows linear with the number of albums/songs.

@SoongNoonien
Copy link
Owner

Hi!
How is the current status? I think 1.2 is solved and 1/1.1 are still valid. We should open a dedicated bug for 1 and 1.1. What about 2? As I said earlier I think this is not how database updates are handled in mpdevil.

Reading this thread again I noticed I forgot to answer one of your questions. Sorry about that.

Can shuffle all tracks or albums be efficiently done with mpdevil? Solving 1. does not necessarily imply this.

Right clicking on "all artists" will open a small menu. Just click on "play" and activate the shuffle mode. This method requires the maximum playlist size (16384) to be at least the number of all your songs. For larger collection this could be a solution, but I've never tried it.

@dgcampea
Copy link
Contributor Author

Sure, feel free to split the issue.
I think 2. isn't too interesting after all, sure it can be done with the idle command but it probably isn't worth spending too much time on it. (I've switched to manual updates too)

About the shuffle, I'll take a look on it. I don't think playing around with mpd config max playlist size will be the answer here (unpredictable behavior depending on different libraries/users).

@SoongNoonien
Copy link
Owner

I've switched to manual updates too

What do you mean by that? I'm still using automatic updates and it is working just fine without the problems you mentioned in the first comment.

About the shuffle, I'll take a look on it. I don't think playing around with mpd config max playlist size will be the answer here (unpredictable behavior depending on different libraries/users).

I think 16384 is now a fixed value in mpd it either is sufficient or not.

@dgcampea
Copy link
Contributor Author

What do you mean by that? I'm still using automatic updates and it is working just fine without the problems you mentioned in the first comment.

Not related to mpdevil. I changed it since it's slightly faster for bulk updating metadata (when editing multiple cuesheets for example) since each file change triggers a db disk write.

About the shuffle, I'll take a look on it. I don't think playing around with mpd config max playlist size will be the answer here (unpredictable behavior depending on different libraries/users).

I think 16384 is now a fixed value in mpd it either is sufficient or not.

It doesn't seem to be fixed according to https://mpd.readthedocs.io/en/latest/user.html#resource-limitations

@SoongNoonien
Copy link
Owner

Not related to mpdevil. I changed it since it's slightly faster for bulk updating metadata (when editing multiple cuesheets for example) since each file change triggers a db disk write.

Ok, I see.

It doesn't seem to be fixed according to https://mpd.readthedocs.io/en/latest/user.html#resource-limitations

Ok, that is weird it is not mentioned in the man mpd.conf anymore. But luckily this is not very important for mpdevil for it is a user decision which should not be changed by the client.

So I'll close this issue. Feel free to open a new issue if you find anything broken related to the shuffling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants