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 with several ANSI sequences #8283

Closed
ZEDCWT opened this issue Nov 15, 2020 · 3 comments · Fixed by #8618
Closed

Memory leak with several ANSI sequences #8283

ZEDCWT opened this issue Nov 15, 2020 · 3 comments · Fixed by #8618
Assignees
Labels
Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@ZEDCWT
Copy link

ZEDCWT commented Nov 15, 2020

Environment

Windows build number: 10.0.19042.508
Windows Terminal version (if applicable): 1.5.3142.0

Any other software?
Node v15.2.0

Steps to reproduce

  • Save the following content in a file, say, name it with test.js
var readline = require('readline'),count = 0;
setInterval(() =>
{
	console.log(++count,new Date)
	readline.moveCursor(process.stdout,0,-1)
},10)
  • Open the cmd.exe in the terminal
  • Run the script saved above with path\to\node.exe path\to\test.js

Expected behavior

No significant memory increasing should be observed

Actual behavior

To compare with, another cmd.exe is also directly started by Win+R and run the same script.
Here is the initial memory usage
Initial memory usage
Also, reported by Process Explorer, the WS Private and Working Set are

WindowsTerminal.exe	36976 K	101816 K

Then, leave them run for hours, and here is the memory usage after the counter hit 400k+.
Memory usage after 400k+

Next, Ctrl+C to end both node processes, and wait for 5min.
Memory usage after node killed

WindowsTerminal.exe	96444 K	157840 K

It seems that it memory leaked.
And by roughly counting during the progress, the memory usage grow about 0.1 MB / 10 sec.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 15, 2020
@DHowett
Copy link
Member

DHowett commented Nov 15, 2020

Hmm, you're right. Interesting.

@DHowett
Copy link
Member

DHowett commented Nov 15, 2020

Good catch!

@DHowett DHowett added Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Nov 15, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 15, 2020
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 16, 2020
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Nov 16, 2020
@zadjii-msft zadjii-msft self-assigned this Nov 23, 2020
@ghost ghost added the In-PR This issue has a related PR label Jan 4, 2021
@ghost ghost closed this as completed in #8618 Jan 4, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 4, 2021
ghost pushed a commit that referenced this issue Jan 4, 2021
Fix memory leak that occurs from not dispatching the end of sequences on all actions (since it is buffering up all characters for trace reasons.) Also don't bother storing if no one is listening.

## PR Checklist
- [x] Closes #8283
* [x] Fixes leak found while bumbling around.
* [x] I work here.

## Detailed Description of the Pull Request / Additional comments
- We trace all the things leading up to the Action phase in the VT parser for ETW tracing to make debugging the parser easier, but we made two mistakes.
- At some point, three of the actions (related to print/execute) weren't dispatching the stored up sequence to tracing and not clearing it. So printing/executing in a giant run over and over caused the vector to bloat and bloat and bloat forever.
- We're storing things even when no one is listening. That's a waste.

## Validation Steps Performed
- Watched it grow every time I did `type big.txt` under `taskman.exe`. Then watched it not do that after.
- I did technically WPR it to figure out this was the culprit.
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8618, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
Fix memory leak that occurs from not dispatching the end of sequences on all actions (since it is buffering up all characters for trace reasons.) Also don't bother storing if no one is listening.

## PR Checklist
- [x] Closes microsoft#8283
* [x] Fixes leak found while bumbling around.
* [x] I work here.

## Detailed Description of the Pull Request / Additional comments
- We trace all the things leading up to the Action phase in the VT parser for ETW tracing to make debugging the parser easier, but we made two mistakes.
- At some point, three of the actions (related to print/execute) weren't dispatching the stored up sequence to tracing and not clearing it. So printing/executing in a giant run over and over caused the vector to bloat and bloat and bloat forever.
- We're storing things even when no one is listening. That's a waste.

## Validation Steps Performed
- Watched it grow every time I did `type big.txt` under `taskman.exe`. Then watched it not do that after.
- I did technically WPR it to figure out this was the culprit.
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants