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

RichText leaks memory when replacing segments #4723

Closed
2 tasks done
prokher opened this issue Mar 14, 2024 · 6 comments
Closed
2 tasks done

RichText leaks memory when replacing segments #4723

prokher opened this issue Mar 14, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@prokher
Copy link

prokher commented Mar 14, 2024

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

Changing RichText content by invoking RichText.ParseMarkdown leads to linear growth of memory consumption. In the practical case I faced it took gigabytes of RAM in a couple of hours.

How to reproduce

To reproduce — call RichText.ParseMarkdown many times with different content. Or just use the snippet I attached below.

Screenshots

Memory growth (one line per second):
image

Example code

package main

import (
	"fmt"
	"runtime"
	"time"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/widget"
	"github.com/google/uuid"
)

func main() {
	a := app.New()
	w := a.NewWindow("Fyne's RichText Memory Leak")
	w.Resize(fyne.NewSize(128, 128/1.618))
	rt := widget.NewRichText()
	go func() {
		var m runtime.MemStats
		for {
			time.Sleep(time.Second)
			// Force GC just to make measurements a bit more precise.
			runtime.GC()
			// Report memory consumption.
			runtime.ReadMemStats(&m)
			fmt.Printf(
				"Sys: %v MiB\tHeapAlloc: %v MiB\tTotalAlloc: %v MiB\tNumGC: %v\n",
				m.Sys/1024/1024, m.HeapAlloc/1024/1024, m.TotalAlloc/1024/1024, m.NumGC,
			)
		}
	}()
	go func() {
		for {
			rt.ParseMarkdown(uuid.NewString())
		}
	}()
	w.SetContent(rt)
	w.ShowAndRun()
}

Fyne version

2.4.4

Go compiler version

1.22.1

Operating system and version

macOS Sonoma 14.4

Additional Information

No response

@prokher prokher added the unverified A bug that has been reported but not verified label Mar 14, 2024
@prokher
Copy link
Author

prokher commented Mar 14, 2024

JFYI, pprof's top shows the following:

(pprof) top
Showing nodes accounting for 611.03MB, 95.87% of 637.35MB total
Dropped 62 nodes (cum <= 3.19MB)
Showing top 10 nodes out of 77
      flat  flat%   sum%        cum   cum%
  157.25MB 24.67% 24.67%   182.25MB 28.60%  fyne.io/fyne/v2/internal/cache.SetFontMetrics
  134.01MB 21.03% 45.70%   134.51MB 21.10%  fyne.io/fyne/v2/widget.(*markdownRenderer).Render.func1
  121.01MB 18.99% 64.69%   121.01MB 18.99%  fyne.io/fyne/v2/canvas.NewText
  106.75MB 16.75% 81.44%   231.77MB 36.36%  fyne.io/fyne/v2/widget.(*RichText).cachedSegmentVisual
      44MB  6.90% 88.34%   474.03MB 74.37%  fyne.io/fyne/v2/widget.(*textRenderer).Refresh
   25.50MB  4.00% 92.34%    25.50MB  4.00%  fyne.io/fyne/v2/internal/cache.(*expiringCache).setAlive
       7MB  1.10% 93.44%        7MB  1.10%  fyne.io/fyne/v2/theme.lightPaletColorNamed
    5.57MB  0.87% 94.31%     5.57MB  0.87%  github.com/yuin/goldmark/parser.NewParser
    5.43MB  0.85% 95.16%     5.43MB  0.85%  github.com/go-text/typesetting/opentype/loader.(*Loader).findTableBuffer
    4.50MB  0.71% 95.87%     6.50MB  1.02%  github.com/go-text/typesetting/harfbuzz.NewFont

@dweymouth
Copy link
Contributor

Is this confirmed to be an actual memory leak (IE the memory consumed by the app does not go down after it's idle for awhile)? If not, I wouldn't really call this a leak, it's just that markdown parsing results in a fair amount of allocations and work for the GC. However, there are some future improvements being planned for the internal font caching which should hopefully result in reduced allocations and steady-state memory use once implemented.

@prokher
Copy link
Author

prokher commented Mar 19, 2024

Is this confirmed to be an actual memory leak (IE the memory consumed by the app does not go down after it's idle for awhile)?

It never goes down. Run this to check yourself:

package main

import (
	"fmt"
	"runtime"
	"time"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/widget"
	"github.com/google/uuid"
)

func main() {
	a := app.New()
	w := a.NewWindow("Fyne's RichText Memory Leak")
	w.Resize(fyne.NewSize(128, 128/1.618))
	rt := widget.NewRichText()
	go func() {
		var m runtime.MemStats
		for {
			time.Sleep(time.Second)
			// Force GC just to make measurements a bit more precise.
			runtime.GC()
			// Report memory consumption.
			runtime.ReadMemStats(&m)
			fmt.Printf(
				"Sys: %v MiB\tHeapAlloc: %v MiB\tTotalAlloc: %v MiB\tNumGC: %v\n",
				m.Sys/1024/1024, m.HeapAlloc/1024/1024, m.TotalAlloc/1024/1024, m.NumGC,
			)
		}
	}()
	go func() {
		for i := 1; i != 1000000; i++ {
			rt.ParseMarkdown(uuid.NewString())
		}
	}()
	w.SetContent(rt)
	w.ShowAndRun()
}

@montovaneli
Copy link
Contributor

montovaneli commented Apr 4, 2024

I also noticed this issue while building a chat app, which led me to many markdown updates due to the message timestamp I decided to update on the screen.

Upon closer examination, I saw that the RichText's visualCache map does grow indefinitely. However, I didn't quite understand the reason for keeping the cache of previous segments if we are completely changing the content via ParseMarkdown.

richmarkdowndebug.mp4

Then I decided to change the ParseMarkdown function in widget/markdown.go to clear the cache if it already exists for that RichText:

func (t *RichText) ParseMarkdown(content string) {
	t.cacheLock.Lock()
	if t.visualCache != nil {
		t.visualCache = nil
	}
	t.cacheLock.Unlock()

	t.Segments = parseMarkdown(content)
	t.Refresh()
}

The memory usage significantly decreased and I was able to watch the GC working (HeapAlloc decreasing after some time). My app was not affected (as far as I can tell).

Anyway, I can by no means claim that this is a solution or even that the visualCache causes a memory leak.

@dweymouth dweymouth changed the title RichText leaks memory on repetitive calls to ParseMarkdown RichText leaks memory when replacing segments Apr 5, 2024
@dweymouth
Copy link
Contributor

Good find. Looking at the code, the visualCache definitely leaks memory as nothing is ever cleared out of it. The Segments property is an exported slice so user code can directly update it and call Refresh so the RichText widget won't have a way to track the lifetime of a segment. We will likely need to add a cleanup task to the RichText widget that runs periodically to clear everything from the visualCache that is not currently displayed. Or do it at every Refresh but it might be a bit expensive to do all the time.

@dweymouth dweymouth added bug Something isn't working and removed unverified A bug that has been reported but not verified labels Apr 5, 2024
@andydotxyz andydotxyz added this to the D fixes (v2.4.x) milestone Apr 5, 2024
@andydotxyz
Copy link
Member

Fixed on develop and I'm cherry-picking to v2.4.5 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants