Improvement: don't queue redundant recalcs

[georgemac-labs]

Hi maintainers, I have been experiencing the UI responsiveness problem described here – i.e. PP becomes laggy when downloading quotes.

After a little analysis, I have a couple of ideas for improvements.

The lowest hanging fruit I see: when new price data is downloaded, that causes a view recalculation to be queued, even if there’s already a recalculation in the queue. And that can happen repeatedly. So… Aren’t we building a queue of redundant work here?

UI_PROGRESS_UPDATE_INTERVAL does limit the damage by reducing the number of items added to the queue, but nonetheless it seems the current logic can burn CPU and slow down the app to do work that’s already been done once or more?

So this PR simply stores whether there’s already an update pending. If true, don’t add to the queue, effectively limiting the queue to only one item (from price updates).

In my tests, this change seems to make the app more responsive while updating quotes.

Please note: I’m NOT confident I understand all the moving parts here, so apologies if I’ve overlooked something and this change doesn't make sense.

[buchen]

@georgemac-labs great idea. I was thinking of a way to reduce it, but this seems like a neat idea.

Is there a particular reason why you run the markDirty now with Display.getDefault().asyncExec?

Did you notice a difference with your data? Because with my file and on MacBook M1, the 300ms are more than enough to recalculate. Therefore I am thinking to increase this to 500ms. I also implemented 874eaf5 (still behind an experimental feature flag) that delays the update of a view if the user is currently editing a cell.

[georgemac-labs]

@buchen

Is there a particular reason why you run the markDirty now with Display.getDefault().asyncExec?

It's what Claude did ;) But I think I understand the reason. If you don't call it async, this logic...

portfolio/name.abuchen.portfolio.ui/src/name/abuchen/portfolio/ui/editor/ClientInput.java

Lines 662 to 669 in 2cd11f2

	if (Display.getDefault().getThread() == Thread.currentThread())
	{
	setDirty(true, recalculate);
	}
	else
	{
	Display.getDefault().asyncExec(() -> setDirty(true, recalculate));
	}

... runs setDirty() async, meaning markDirty() returns immediately, and isModelUpdatePending gets set to false, when the update is actually not completed. That would mean more updates could be queued, and the change wouldn't do what it's supposed to.

Having dug into this, I will say that the variable name seems suboptimal, and also my description above is slightly off. The state that is stored is actually isModelUpdateInFlight – i.e. the update can be pending OR actively being processed. Could rename this if you like.

But the distinction is immaterial to the logic, because markDirty() is called again from the finally, so all updates will be processed in the end.

Did you notice a difference with your data?

Yes, as mentioned above: app is more responsive while updating quotes.

Because with my file and on MacBook M1, the 300ms are more than enough to recalculate. Therefore I am thinking to increase this to 500ms

I can only say for me personally: I view price updates as an async process from a UX perspective. I know I'm going to have to wait a short while. I'm not concerned about making them a little faster, or having the views update instantaneously. My only concern is that the app remain responsive during updating, so I can click around a bit.

[georgemac-labs]

@buchen having understood the code better, I'm a little dissatisfied with the resulting logic.

• We now have two separate mechanisms to limit recalcs – batching (UI_PROGRESS_UPDATE_INTERVAL) and anti-backlog (this change)
• Because the check is actually for update in flight, not pending, you have a case where new quote data has arrived, isModelUpdatePending is true (update not 100% completed), but actually it's too late – that security has already been recalculated with the old data. It will be recalculated by the next loop or the finally block, but if the updates are not stringently triggering recalcs, I wonder why even bother linking the two. We could just simplify the whole thing and use a timer.
• On reflection, my idea would be a variant of the current logic: fixed delay between pushing updates, rather than a fixed rate. Because then the app should always manage to respond to the user, no matter what the spec of the machine and the amount of data. And as mentioned, my personal perspective is I don't care if the views are a couple of seconds behind. Responsiveness matters.
• That said, I don't think any of this is really important, and maybe not worth more time. This PR is a bit illogical, but works OK in practice. So I'd be happy to either implement the delay approach or leave this as is – up to you.

[buchen]

We could just simplify the whole thing and use a timer.

I am not sure if I understand your statement. Isn't the code using a timer?

On reflection, my idea would be a variant of the current logic: fixed delay between pushing updates, rather than a fixed rate.

Right now the time is triggering every 300ms. And the two things happen:

• the progress bar is updated (and the update view - but that is "cheap")
• the calculations are retriggered (but only if the model is dirty - maybe there are no new prices)

I understand your statement of separating the two. And you suggest: not check every 300 ms, but check 300 ms after the last recalculation was finished (of course we can also change the 300 ms to something else).

I think that could remove the update pressure. We would still need the "timer" for the progress bar. Happy to entertain a pull request on this.