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

[Improvement] Async bottle loading #574

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vcsoares
Copy link

What?

This PR makes the methods responsible for loading and updating a bottle's program list run asynchronously. It also adds indicators in the Bottle and Program views to make the user aware there's a loading operation in progress.

Why?

With relatively large bottles, loading the programs list can sometimes take a long while. Whisky used to do this synchronously, which in turn would lock the app until the operation finished, leading to degraded performance every time you switched bottles or changed pinned programs, as well as slow startup times.

How?

By moving execution of methods that load a bottle's program list to a dedicated DispatchQueue and adding a lock to the Bottles struct to protect accesses as needed, since there's now a chance for concurrent load operations to happen. Besides that, calls to updateInstalledPrograms() have been reduced, and this method will only run if the programs list for the current bottle is empty. This means that pinning/unpinning programs in the programs list won't trigger a full reload, for instance.
UI affordances were implemented using common SwiftUI components.

image
image

programs.append(contentsOf: favourites)
programs.append(contentsOf: nonFavourites)
private func sortPrograms() {
DispatchQueue(label: "whisky.lock.queue").async {
Copy link
Member

Choose a reason for hiding this comment

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

In general, we don't use GCD in Whisky, using structured concurrency would be preferable.

Copy link
Author

Choose a reason for hiding this comment

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

I opted for using a DispatchQueue in this specific scenario to ensure bottle loading operations get queued up in the same thread. Structured concurrency would not have this guarantee (at least as far as I know), and in general I avoided going down the route of rewriting Bottle to be an Actor.

Copy link
Member

Choose a reason for hiding this comment

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

Actor based approach might be preferred here.

pin: pin,
loadStartMenu: $loadStartMenu,
path: $path)
ZStack {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this ZStack is needed. Why block the user from accessing loaded pins until they've all loaded?

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm, I see what you mean - yeah, maybe a less fancy but less obstructive way of displaying the indicator would be better indeed. I'll change that, thanks for the feedback :)

@IsaacMarovitz
Copy link
Member

@vcsoares Rebase needed after recent reorganisation

@vcsoares
Copy link
Author

Hi - just chiming in that life got a little hectic these weeks and I haven't had much time to resume work on this. As soon as I get some spare time I'll address your comments @IsaacMarovitz. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants