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

Reduce CPU consumption of Entry cursor by around 80% #4560

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

andydotxyz
Copy link
Member

Description:

Reduce CPU load by around 80% by fading over only 1/5th the time.
Also avoid re-calculating MinSize when the visible content has not changed.

Fixes #1946, #4156

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@dweymouth dweymouth added the optimization Tickets that could help Fyne apps run faster label Jan 24, 2024
widget/entry.go Outdated
Comment on lines 488 to 494
func (e *Entry) Refresh() {
e.propertyLock.Lock()
e.minCache = nil
e.propertyLock.Unlock()

e.BaseWidget.Refresh()
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying that it need to be done now or should be done, but maybe it would make sense to have some kind of MinSize cache built in to the basewidget? I suppose it is something we might want to do for all widgets more or less (unless the computation is very cheap).

Copy link
Member Author

Choose a reason for hiding this comment

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

I did propose the BaseWidget inclusion some time back but there was pushback for a reason I cannot recall.
Let's get it in here and see how useful it is and it could be pulled up without impact later.

Copy link
Member

Choose a reason for hiding this comment

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

It might be worth exploring again. Either as part of the base widget or as some kind of ready cache-system that developers can add to their widgets (maybe using async.Size internally?). We should at least look at caching MinSize in more widgets :)

@andydotxyz andydotxyz merged commit caf50f6 into fyne-io:develop Jan 26, 2024
11 checks passed
@andydotxyz andydotxyz deleted the fix/cursorcpu branch January 26, 2024 22:50
@dweymouth dweymouth added this to the v2.4.5 milestone Feb 19, 2024
@dweymouth
Copy link
Contributor

This should be pulled into v2.4.5 if possible as it's another huge optimization, but like the one for animation runner, it may need minor reworking to not use async.Size and just use a regular fyne.Size with locking instead

@Jacalz
Copy link
Member

Jacalz commented Feb 19, 2024

It might be worth just waiting for v2.5.0 if the changes are as big as they are

@dweymouth
Copy link
Contributor

It might be worth just waiting for v2.5.0 if the changes are as big as they are

I think it might just be undoing the last commit 42ab4dd

Plus, if the new threading model is landed in 2.5, which is the current plan, async.Size may not even be needed anymore as widgets can assume their API calls would arrive from a single thread

@dweymouth
Copy link
Contributor

This is a huge optimization for any Fyne app where the user spends any non-trivial amount of time in text entry. I think we really should get it in 2.4.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Tickets that could help Fyne apps run faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants