bunqCommunity / bunqDesktop

The unofficial, free and open source desktop application for the bunq API
https://bunqdesk.top
MIT License
272 stars 53 forks source link

App sometimes feels slow and unresponsive #396

Open Cheaternl opened 5 years ago

Cheaternl commented 5 years ago

After starting the app (and logging in), it feels slow and sometimes unresponsive. Resizing the app (AFTER logging in) seems to block/hang the application, but after half a minute, the app DOES resize to fullscreen. Resizing before logging in isn't slow. This may be related to the sync process in the start?

Also, after logging in, I cannot click any account, even when the payments on the right are loaded already. After syncing is done (1347 events), clicking an account doesn't apply directly.

I have, in total, 21 accounts and 1059 payments and requests. Do you (as a builder) have at least that amount of records so you can 'test' how the application feels? I have a core i 7 with 16gb of memory and my system feels VERY responsive and fast for ALL other applications.

I resetted the bunq application in the settings screen, just to see if this aided the slowness issues.

Crecket commented 5 years ago

The next version will have a setting to limit the amount of events per type but it is likely a combination of the amount of events + the amount of requests.

It has to do amount of accounts * 6 requests if I want to check if any of them have new events. Along with that the rate limit is incredibly strict so that is why it takes so long to update right now.

I have sandbox accounts which have thousands of payments to test with and those don't lag as much on the development build so could you test that and see if it's a little better? I'm still improving things and I'm thinking of adding a option to let users ignore certain accounts for every or the initial background sync event

Cheaternl commented 5 years ago

How can I test the development build? Is that the nightly build which I can find under 'releases'? I only have knowledge about how to install THE compiled windows binary installation file from your releases tab in Github.

Crecket commented 5 years ago

The nightly build is close to the development version since it gets built every night automatically ๐Ÿ‘

Crecket commented 5 years ago

Also another important thing to keep in mind is that using the "All" event count on the main dashboard page is a major performance issue when you have a couple of hundred events.

Cheaternl commented 5 years ago

Than the nightly build didn't speed up things for me (tested on 2 computers).

(Also, the nightly build doesn't show the arrow button on the account, when clicked (and filtered). The normal release version does show this.) As it is a nightly build, I didn't report this in a new/seperate ticket.

Cheaternl commented 5 years ago

"Also another important thing to keep in mind is that using the "All" event count on the main dashboard page is a major performance issue when you have a couple of hundred events."

Bold suggestion: Would you consider storing and caching - ALL - payments from all accounts to the local PC, in an encrypted way (e.g. via the API key or so), on the local machine, and NEVER refetch these payments, except when the user really wants to (like, a 'total sync' button (or by pressing Shift while clicking the sync button)) . I think that payments older than 2 months will not be changed anymore by the Bunq ecosystem, so storing them locally wouldn't be a large risk for users seeing 'old' / 'inaccurate' information for these payments. While storing these payments, the application fetch algorithm can be optimized to go back only that far where the cached items begin.

Cheaternl commented 5 years ago

This suggestion also applies in that other reported issue of mine, where you say that 1000 events are loaded at max.

Crecket commented 5 years ago

It already stores the payments on the account right now, I'm going to change it so that it only loads all events once and then gets the 200 most recent ones foreach account when you sync to update the most recent ones.

The current system updates everything since I won't know if something was updated before the first 200 events for each type otherwise since bunq doesn't have a system to get update for non-server apps

OGKevin commented 5 years ago

The current system updates everything since I won't know if something was updated before the first 200 events for each type otherwise since bunq doesn't have a system to get update for non-server apps

Canโ€™t you maybe use the newer Id? (Pagination on the listing, the Iโ€™d of the the last pulled object) and only fetch new entries. Untill bunq opens that stupid endpoint.

Crecket commented 5 years ago

If I do that and one of the existing payments gets updated/canceled or something then it won't show up in bunqDesktop. I do think I can maybe decrease the count parameter to see if that improves the API load times and improve the merging functions afterwards

Cheaternl commented 5 years ago

I agree with Crecket, at least a bunch of the last payments should be synced, but certainly not ALL. When starting the BunqDesktop, I hope it shouldn't have to fetch ALL payments from my Bunq Account, because that's a performance-hit and results to decissions like "only fetch 1000 events/payments". I understand you want to make this 1000 number configurable, but I'm not waiting for a setting that makes things worse when I raise the number.

I use BunqDesktop because it gives me QUICK access to ALL my payments. I want to know how much money I have within ONE account for a specific budget (which I can denote by categories). I have categorized ALL payments in that accounts, so I can filter on them and know the total IN, and total OUT of the money, (and the total change).

As BunqDesktop is the only tool that can show me these numbers, and is now non-working for mee (1000 events max and slow), this brings me to a decission that I have to create more accounts for each single budget I want to create in my life. At the moment I have multiple budgets on one single account.

Crecket commented 5 years ago

You can now set the limit to only load the last 50 events per account/type and the queue system is a bit more optimized now when loading a lot of data

Cheaternl commented 5 years ago

And what does it do when starting the application? Only load and show me 50 events? Because I'm only interested in the FULL list of all payments for each account. How do I do that?

Crecket commented 5 years ago

It will load 50 payments for each account and each event type. That way less data is stored in memory while the queue is active. There is no decent way to get all events for all accounts in a performant way OR reasonably quickly way with the current API when you have more than 3 accounts

Crecket commented 5 years ago

And I noticed you talked about dates and such, the API doesn't allow me to set a specific date range or a from/to filter or anything like that.

I can only grab the ID of a payment and then fetch 1-200 older or newer payments than the ID I give it. That is why you might have 1 account with more payments which only goes back a few weeks whereas other accounts with less payments show everything.

That is why I hope bunq will realize how obnoxious the current system is for applications where you want to see all events for your accounts. This would all be solves instantly if we got access to the private /events endpoint

Cheaternl commented 5 years ago

So if I'm correct, Bunq expects an application to hold the last/newest event/payment ID that the application fetched last time. With this ID, the application can fetch newer events/payments and update the info in the application. That sounds quite a reasonable good way of getting up-to-date for a Bunq account.

But I didn't remember if you said that the BunqDesktop application DOES store all payment/event data in a local storage/database, or it doesn't? Because, if it doesn't, I understand why such an endpoint with relative data (1-200 newer/older events) is not the easiest way to work around.

Crecket commented 5 years ago

The problem is that this ID is per monetary account and they have a 3 second rate limit per request. So if I want to get new payments and update any existing ones I have to send 1 request for each event type (6) per monetary account. So in your case that is 120+ requests just to check if anything changed or new events happened.

If they allowed access to the private /events endpoint I could do 3 requests to that endpoint and get ALL events for ALL accounts in less than a second instead of the 1 minute+ that you have to wait

Cheaternl commented 5 years ago

What's the endpoint name what you speaking of?

Crecket commented 5 years ago

It isn't available on the public API, it is only used internally (Which is why you don't really notice long load times in the bunq app)

Cheaternl commented 5 years ago

I mean, what's the endpoint you're using now for fetching events/payment overview data.

Crecket commented 5 years ago

It looks like they might be adding the events endpoint ๐ŸŽ‰

https://github.com/bunq/sdk_php/commit/1ca394e69959a25faf95e56d499daa6ed567da4f#diff-4b8e59be4e722c1ecd87072720eb74c8

Cheaternl commented 5 years ago

Great!! Now I have confidence that you can solve the slowness problems, without reducing the amount of data BunqDesktop will show by default.

Time to shine! :+1:

Crecket commented 5 years ago

I've added some decent improvements but I'm still not entirely happy so I'm moving this to the next release. When bunq fixes the event endpoint this will likely be fixed instantly ๐Ÿ˜ข

Cheaternl commented 5 years ago

Thank you! I admire that you don't ignore this issue.

Crecket commented 5 years ago

I'm working on an experimental branch which implements IndexedDb instead of localstorage. Localstorage was fine for smaller amounts of data but storing hundreds of events like it does now isn't as performant.

Since IndexedDb is asynchronous as well it should reduce some of the unresponsiveness when the main thread is trying to read/write data with the current localstorage setup.

Cheaternl commented 5 years ago

Async and the usage of a real DB are 2 great pillars to achieve speed increase. Good Job.

Crecket commented 5 years ago

Moving this to the next release so we can run a hotfix for the update 10 changes

Cheaternl commented 5 years ago

Alright !

Wouldn't the 'hotfix release' interfere with your release schema? In other words: "the hotfix release is done, between the normal planned releases" ?