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

Memory leak in fontMetrics cache on desktop driver #4010

Closed
2 tasks
longkui-clown opened this issue Jun 29, 2023 · 9 comments
Closed
2 tasks

Memory leak in fontMetrics cache on desktop driver #4010

longkui-clown opened this issue Jun 29, 2023 · 9 comments
Labels
bug Something isn't working optimization Tickets that could help Fyne apps run faster

Comments

@longkui-clown
Copy link

longkui-clown commented Jun 29, 2023

Checklist

  • I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
  • This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

  • When I use widget.NewMultiLineEntry with Entry.SetText() function as my app log area, there appera a memory leak
  • I tried to comment out the SetText function and the memory leak was relieved。

How to reproduce

  • Just run the code in this issue, there is no pprof code, and it can reproduce

Screenshots

No response

Example code


  • Code with pprof
package main

import (
	"fmt"
	"net/http"
	_ "net/http/pprof"
	"strings"
	"time"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/container"
	"fyne.io/fyne/v2/widget"
)

var logs = []string{}
var maxLogTake = 50

func main() {
	go func() {
		http.ListenAndServe("0.0.0.0:10000", nil)
	}()
	app := app.New()
	win := app.NewWindow("Hello World!")
	logArea := widget.NewMultiLineEntry()
	logArea.SetMinRowsVisible(15)

	content := container.NewVBox(
		container.NewHBox(
			widget.NewLabel("Click the button"),
			widget.NewButton("Log Test", func() {
				s_now := time.Now().Format("2006-01-02 15:04:05")
				for i := 0; i < 50; i++ {
					logs = append(logs, fmt.Sprintf("[Log](%v): %v", s_now, i))
				}

				logLen := len(logs)
				if logLen > maxLogTake {
					logs = append(logs[0:0], logs[logLen-maxLogTake:]...)
					logLen = maxLogTake
				}

				s_logs := strings.Join(logs, "\r\n")
				logArea.SetText(s_logs)
				logArea.CursorRow = logLen
				logArea.Refresh()
			}),
		),
		logArea,
	)

	win.Resize(fyne.NewSize(450, 320))
	win.SetContent(content)
	win.ShowAndRun()
}
  • click some times
    click1

  • click some times again
    click2

  • And can see the memory is increasing, I run again, and same

Fyne version

2.3.5

Go compiler version

1.20.2

Operating system and version

Windows 10

Additional Information

No response

@longkui-clown longkui-clown added the unverified A bug that has been reported but not verified label Jun 29, 2023
@longkui-clown longkui-clown changed the title When I use widget.NewMultiLineEntry with Entry.SetText() function as my app log area, there appera a memory leak When I use widget.NewMultiLineEntry with Entry.SetText() function as my app log area, there appear a memory leak Jun 29, 2023
@longkui-clown
Copy link
Author

  • I use debug and find that C:\Users\long\go\pkg\mod\fyne.io\fyne\[email protected]\widget\richtext.go function cachedSegmentVisual, line 205, there is t.visualCache, and debug find that this cache will cache continue, and keeped similer item, seemly is bug, i'm continue debugging..
  • Here is screenshot
    richtext
    cache

@andydotxyz
Copy link
Member

I believe this relates to text caches operating at a line level instead of per-glyph rendered. However you should be aware that everything drawn ok screen does get cached, so continually setting more text will increase memory used.

@longkui-clown
Copy link
Author

yes, this design is perfect, while maybe someone don't know this design, and use it like me, will going to end up in a situation similar,I think there should have a switch function to control if use the cache, when use SetMinRowsVisible, it seem like no need to cache so many items

@andydotxyz
Copy link
Member

Turning off texture caching means calculating and drawing text on every single GPU frame, it's a very bad idea.

I think what you want here is for Entry to be smarter to not have to draw all its content. At this time that's planned but not added. Realistically an editor widget for a log dump isn't ideal, a List of Labels would be a lot, lot faster as list contains the types of optimisation I mentioned.

@Goltanju
Copy link
Contributor

Goltanju commented Jul 3, 2023

Apologies for interrupting here but I actually use a multiline entry for a log listing and other listings on occasion as well. One of the big reasons I do this is because there's no way for a user to copy text from a label. For some cases, I'll make a big label at first and the user has to click a button (or menu item, etc.) to switch to edit mode so they can copy, which is a bit hacky and let's the user paste where I don't really want them to, but.

@andydotxyz
Copy link
Member

You'll notice that in many apps a "copy button" alongside a commonly-copied line of text is a better user experience. Google, GitHub and others have started doing this for security codes and code snippets etc. Interacting with text to copy it out from an Entry isn't really the optimal user experience unless you are literally in a text editor in my opinion.

@Goltanju
Copy link
Contributor

Goltanju commented Jul 3, 2023

How would you implement a log viewer using Fyne? Copying the whole log usually isn't what a user wants. Copying a single line is probably a 90% solution, but a copy button for every line seems hacky and it doesn't handle the other 10% (made up percentages haha) where the user wants to grab several log lines.

Another example is a note taking app, think Evernote and the like. Here, 90% of the time the user will just be viewing notes and copying parts of them here and there as they need them elsewhere. 10% of the time they'll be editing. Having a read-only yet interactable view of a note would be huge here. Right now I show a read-only version, but they can't copy from it like they could on a web page, for example. They have to enter edit-mode just to copy, or I could leave it always in edit-mode which offers up new problems, haha.

Much of this pushes towards making these particular things webapps, but I really like Fyne! haha

@andydotxyz
Copy link
Member

Disabled Entry is read only but selectable.
Typically I would say a lot viewer copy a line would be ok - with export to file for the whole lot I guess.

At some point we will manage to optimise Entry to handle these huge texts, using similar techniques that have been polished in list/table/tree.

@longkui-clown
Copy link
Author

  • I will try another way

  • And debug find another cache many position
    cache_another

@dweymouth dweymouth changed the title When I use widget.NewMultiLineEntry with Entry.SetText() function as my app log area, there appear a memory leak Memory leak in fontMetrics cache on desktop driver Sep 11, 2024
@dweymouth dweymouth added bug Something isn't working optimization Tickets that could help Fyne apps run faster and removed unverified A bug that has been reported but not verified labels Sep 11, 2024
@dweymouth dweymouth added this to the E fixes (v2.5.x) milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working optimization Tickets that could help Fyne apps run faster
Projects
None yet
Development

No branches or pull requests

4 participants