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

Leakage caused by Stacktrace #143111

Closed
alexdima opened this issue Feb 15, 2022 · 4 comments
Closed

Leakage caused by Stacktrace #143111

alexdima opened this issue Feb 15, 2022 · 4 comments
Assignees
Milestone

Comments

@alexdima
Copy link
Member

alexdima commented Feb 15, 2022

I noticed this while investigating #142933

  • curl https://raw.githubusercontent.com/zemirco/sf-city-lots-json/master/citylots.json >citylots.json
  • open the file
  • close all editors
  • take a heap snapshot
  • observe a 300KB minimap Uint8ClampedArray that is being referenced via an Error stored in a Stacktrace in a Listener.
  • Heap-20220215T162359.heapsnapshot.zip
image
@alexdima
Copy link
Member Author

@jrieken Because this might be caused by a recent refactoring in 4d43287, I have pushed a change via 8dcfb37 to disable the emitter leak monitor for now.

@jrieken @bpasero Now that I saw how the monitor works (it is always enabled), and that 4d43287 tries to avoid reading Error.stack (presumably to avoid a perf hit), we might consider enabling the leak monitor only behind a --troubleshoot CLI flag.

@jrieken
Copy link
Member

jrieken commented Feb 21, 2022

We do this since a couple of years, what has changed is the lazy creation of the actual string (by holding a reference to the error). We can turn that back to be a string, esp. since the stacktrace value is used just after its creation. @alexdima I see how this consumes memory (more than before) but I fail to see leak here. Can you explain?

@alexdima
Copy link
Member Author

alexdima commented Feb 21, 2022

Using the original steps, there is now a reference to an array buffer, kept alive by a minimap instance, kept alive by an editor view instance, which is kept alive by something called StackFrameInfo, which I guess is an internal v8 class. All these objects are logically disposed, the original steps involve closing the editor.

Of note is that the editor core does not have any listeners to IKeybindingService.onDidUpdateKeybindings so I cannot explain why the _listeners list of IKeybindingService._onDidUpdateKeybindings would end up referencing an editor view instance.

Do you think that this leak always existed, even when reading Error.stack eagerly? What change should I try locally to simulate the old behavior?

jrieken added a commit that referenced this issue Feb 21, 2022
jrieken added a commit that referenced this issue Feb 21, 2022
…he first 30 listener for leak checks, some lipstick, #143111
@jrieken jrieken closed this as completed Feb 21, 2022
@jrieken
Copy link
Member

jrieken commented Feb 21, 2022

Went back to eagerly getting the value of a stacktrace via new Error().stack. Still a mystery why the StackFrameInfo hold onto "random" thing tho.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants