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 when resizing quickly #1857

Closed
kdrag0n opened this issue Jun 10, 2024 · 8 comments · Fixed by #1929
Closed

Memory leak when resizing quickly #1857

kdrag0n opened this issue Jun 10, 2024 · 8 comments · Fixed by #1929
Assignees

Comments

@kdrag0n
Copy link
Contributor

kdrag0n commented Jun 10, 2024

Repro:

  • Try to match the window size shown here, so that the terminal isn't much wider than the xxd output lines
  • Run xxd /dev/zero for a bit to fill the scrollback buffer
  • Stop it with Ctrl-C
  • Repeatedly grow and shrink the window horizontally. Don't wait for reflow to finish; keep resizing it back and forth while it's still busy reflowing for the last resize event.

Unlike #1853, this memory is released when the respective split/tab/surface is closed, but it's not released as long as the surface exists.

All the leaked memory is in raw mmap regions in this case (as shown by footprint), so I think it's caused by leaked Pages.

Scrollback buffer should be large enough that reflow takes a noticeable amount of time.

Screenshot.2024-06-09.at.09.32.31.PM.mp4
@kdrag0n
Copy link
Contributor Author

kdrag0n commented Jun 10, 2024

Slowly but steadily growing OR shrinking in one direction (without waiting for reflow) seems to be the most reliable way to reproduce this.

@kdrag0n
Copy link
Contributor Author

kdrag0n commented Jun 10, 2024

It also helps to have a high scrollback limit to make this more obvious. Mine's set to 1 GB.

@mitchellh mitchellh added the needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. label Jun 10, 2024
@mitchellh
Copy link
Contributor

I tried the easy thing by replacing posix.mmap with std.testing.allocator and hoping the unit tests saw anything weird (since the testing allocator does leak detection). No leaks detected, so it wasn't going to be that easy. 😄 Still looking.

@qwerasd205
Copy link
Collaborator

I cannot reproduce this. @kdrag0n, can you test again on the latest version and see if you still get this behavior? It's possible one of the recent changes fixed whatever was causing this.

@kdrag0n
Copy link
Contributor Author

kdrag0n commented Jun 26, 2024

Hmm, I'm still seeing this on 7741463.

@kdrag0n
Copy link
Contributor Author

kdrag0n commented Jun 26, 2024

Can you try this config and font (just in case it somehow matters)?

font-family = "Twilio Sans Mono"
font-family = "Cascadia Code"
font-style-bold = Semibold
font-style-bold-italic = "Semibold Italic"
font-size = 14

#adjust-cell-height = 5%

# 100 chars * 10 million
scrollback-limit = 1000000000
adjust-cursor-thickness = 200%
cursor-style-blink = false

mouse-hide-while-typing = false
mouse-scroll-multiplier = 1.5

keybind = super+f=write_scrollback_file

window-padding-x = 6
window-padding-y = 6

window-save-state = always

copy-on-select = clipboard

#macos-titlebar-tabs = true
unfocused-split-opacity = 0.8

term = xterm-256color

minimum-contrast = 1.1
#bold-is-bright = true

config-file = theme
window-theme = system

#shell-integration = none

focus-follows-mouse = true

@qwerasd205
Copy link
Collaborator

qwerasd205 commented Jun 26, 2024

I can reproduce this with your config! I'll try to narrow down the exact cause. (My suspicion is that it could be related to copy-on-select or focus-follows-mouse)

@qwerasd205
Copy link
Collaborator

Nevermind... now I can reproduce it without the config... either way I am working on debugging the problem now.

@mitchellh mitchellh added bug and removed needs-confirmation A reproduction has been reported, but the bug hasn't been confirmed or reproduced by a maintainer. labels Jun 29, 2024
@mitchellh mitchellh added this to the 1.0 Public Release milestone Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants