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

WTrackMenu: Cleanup & performance regression fixes (part 2) #2684

Merged
merged 22 commits into from
Apr 21, 2020
Merged

WTrackMenu: Cleanup & performance regression fixes (part 2) #2684

merged 22 commits into from
Apr 21, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Apr 19, 2020

@Be-ing @hacksdump

As promised another round of optimizations for #2612. This one fixes some more regressions and also an indexing bug in a loop in updateMenu() that only considered the first element.

We need to optimize updateMenu() that still is very inefficient and able to freeze the UI longer than acceptable. This was already an open issue before extracting the WTrackMenu class.

The major regression and pain point caused by getTrackPointers() is still unresolved, I added comments. A proof-of-concept for handling those long running actions is ready for testing. Next time ;)

@uklotzde uklotzde added this to the 2.3.0 milestone Apr 19, 2020
@uklotzde uklotzde requested a review from Be-ing April 19, 2020 08:01
@Be-ing
Copy link
Contributor

Be-ing commented Apr 19, 2020

I don't know how to do this in C++, but I am wondering if we could create some sort of asynchronous iterator class that only keeps a single TrackPointer in memory at a time.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 19, 2020

I don't know how to do this in C++, but I am wondering if we could create some sort of asynchronous iterator class that only keeps a single TrackPointer in memory at a time.

I have prepared a small, internal framework for exactly this purpose, part of #2656 (which also includes the pending changes for WTrackMenu). It is not asynchronous, single tracks are still loaded in the UI thread, and all the work is done in the main thread. This is acceptable as long as we provide progress feedback and an option to abort long running operations. It saves us from managing and controlling multiple background tasks and is already a huge improvement compared to the current situation.

@uklotzde
Copy link
Contributor Author

Force pushed, because the last commit did not build.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 19, 2020

I have compared the code in WTrackMenu with that in WTrackTableView. Unfortunately WTrackMenu::updateMenus() is causing a major performance regression by loading all tracks at once instead of obtaining this information from the TrackModel as before. This still needs to be fixed. Fixed

@uklotzde
Copy link
Contributor Author

WTrackMenu::updateMenus() should finally be fixed. This was the most urgent issue.

@uklotzde uklotzde changed the title WTrackMenu: Cleanup & fixes (part 2) WTrackMenu: Cleanup & performance regression fixes (part 2) Apr 19, 2020
src/widget/wtrackmenu.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Apr 21, 2020

Beyond the scope of this PR, but related: I think mass actions get slowed down a lot by writing to file tags. This is unnecessary when I clear analysis settings right before reanalyzing. Maybe we can come up with a way to improve this.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 21, 2020

The code looks good to me. I will test it after my ongoing reanalysis finishes.

@uklotzde
Copy link
Contributor Author

Beyond the scope of this PR, but related: I think mass actions get slowed down a lot by writing to file tags. This is unnecessary when I clear analysis settings right before reanalyzing. Maybe we can come up with a way to improve this.

We don't have the facilities for managing queued background tasks. The only exception is the special case multi-threaded batch analysis.

The first step towards a better user experience is to show a modal progress report and the option to abort long running tasks as demonstrated in #2656. Any non-modal extensions are out of scope yet.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 21, 2020

Beyond the scope of this PR, but related: I think mass actions get slowed down a lot by writing to file tags. This is unnecessary when I clear analysis settings right before reanalyzing. Maybe we can come up with a way to improve this.

This could be done with a reanalyze action. That would also require adding a progress bar to the GUI. Then we could finally get rid of the unintuitive Analyze LibraryFeature.

@Be-ing Be-ing merged commit aed93ad into mixxxdj:master Apr 21, 2020
@uklotzde uklotzde deleted the wtrackmenu_cleanup branch April 21, 2020 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants