Skip to content

Conversation

ryanclark
Copy link
Contributor

This reduces the memory usage of the session recording processor by ~80%.

Instead of keeping the entire terminal state (primary and alternate buffers and all the information about the glyphs) to later reconstruct into an SVG, the SVG is generated immediately (as it doesn't have to store all of the same information).

Before (325mb)
before-memory

After (62mb)
after-memory

changelog: Reduces the memory usage when processing a session recording by ~80%

Copy link
Contributor

@okraport okraport left a comment

Choose a reason for hiding this comment

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

LGTM assuming the test is fixed. Nice.

Comment on lines 255 to 257

state := vt.DumpState()
svg := string(VtStateToSvg(&state))
svg := string(VtToSvg(&state))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state := vt.DumpState()
svg := string(VtStateToSvg(&state))
svg := string(VtToSvg(&state))
svg := string(VtToSvg(vt))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, cheers

@espadolini
Copy link
Contributor

Can we reduce the memory usage by another factor of 10 by making maxThumbnails 100, or would that impact UX too much?

@ryanclark
Copy link
Contributor Author

ryanclark commented Oct 15, 2025

Can we reduce the memory usage by another factor of 10 by making maxThumbnails 100, or would that impact UX too much?

I did think 100 might work but after thinking about it more, 100 thumbnails would generate a thumbnail every 10 seconds for a 16 minute recording, which would be big gaps I'd like to avoid (one every 10+ seconds for hour long + is fine)

500 maybe? That would be a gap every 2 seconds for 16 minutes. Your session generated 448 thumbnails and used 62mb memory, the semaphore could limit concurrency to 4 or 8 at a time?

fwiw I'm going to look at uploading the thumbnails as soon as they're generated also

@espadolini
Copy link
Contributor

the semaphore could limit concurrency to 4 or 8 at a time

Especially if we limit the inflight processing it's fine to leave the thumbnail count as is for this PR.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from rosstimothy October 15, 2025 15:10
@ryanclark ryanclark added this pull request to the merge queue Oct 15, 2025
Merged via the queue into master with commit b0555be Oct 15, 2025
40 checks passed
@ryanclark ryanclark deleted the ryan/change-recording-svg branch October 15, 2025 15:43
@backport-bot-workflows
Copy link
Contributor

@ryanclark See the table below for backport results.

Branch Result
branch/v18 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants