vscode: Memory leak

I think it’s quite irresponsible to mark this out-of-scope, this affects all of your users on all platforms.

Front page of HN = should probably fix hint hint 😉

That’s not how open source software works, @sam0x17.

Guys, It’s up to them to decide what is the scope. It’s up to us if we decide we want to use their Software.

They have made their choice. We make our own choices.

Ya, if truth is not constructive. Shut up.

VS code is written on Javascript. Truth is you can’t manage low level memory with it unless you re-engineer entire JS. So… Nothing can be done about it unless Microsoft comes out with a miracle.

Actually, just for completeness sake, I have tried one more time to reproduce and this time found a logical leak. The thing is that the leak only reproduces out of Insiders or Stable and not when running from source. This is due to the difference in product.json and the presence of file based recommendations in the stable/insiders builds. Back in October, I only tried to reproduce running from sources and believed the issue is about the difference in the memory usage reported by the heap snapshot and the OS and the issue was about how v8 is freeing memory from the OS.

But the following does show a clear leak:

There is a listener whenever a text buffer is created that is not disposed around here – https://github.com/microsoft/vscode/blame/c4b7d109123ca612cbe3d40e9c67575a145ec020/src/vs/workbench/contrib/extensions/browser/fileBasedRecommendations.ts#L152-L155 .

@sandy081 we should remove that listener as soon as a text buffer is disposed, and I plan to also “null” out some text buffer members on my side to avoid that forgetting to dispose a listener would cause such a leak in the future.

I am sorry for not noticing the logical leak back in October and thank you for bringing this issue back to my attention, such that I tried reproing again! ❤️

There is “Improve performance, scalability, and security of VS Code and its extensions” in the roadmap. I don’t see how this issue isn’t relevant for that.

@00benallen Electron is owned by the OpenJS Foundation, and the issue isn’t with Electron anyway, it’s in V8 (the underlying JS runtime). And yes, reworking memory allocation in V8 is way out of scope for the VSC team. They have neither the expertise nor the authority to unilaterally change that kind of thing. https://old.reddit.com/r/programming/comments/jzrxzs/vscode_memory_leaks_are_outofscope/gddvzad/

So completely reasonable to mark it out of scope. This is not even a memory leak, it’s allocating memory for later use. Try reopening the file a couple 100 of times and see if the memory increases, then we talk about a memory leak which they could probably fix.

First of all, sorry for causing this bug and added the fix. Here are the details about the bug and fix:

We have file based recommendations (FileBasedRecommendations ) which listen on text model additions to the editor and recommend extensions based on file extension & language. Recently, I added an improvement to this to check for recommendations when user changes the language of the file (more details here - https://github.com/microsoft/vscode/issues/102823). For this, I listen on model’s language change. I was disposing this listener only when FileBasedRecommendations class is disposed which caused this leak as it was holding the model even after the model got disposed.

Fixed it by disposing the model listeners the FileBasedRecommendations has, when model is getting disposed (onWillDispose). To confirm this here are the heap snapshots before and after the fix.

Before

After

Before: There is an increase in memory (~2.6MB) every time I open a large file and close it. After: No hike in memory when I do the same.

Just wanted to add a note here for clarity:

I have investigated this back in October, but I did not find a logical leak in our code. We took two heap snapshots, one taken before opening a very large file and the other one taken after closing the file. The logical JS memory usage did not grow (according to the Heap Snapshots). That is why I linked to https://chromium.googlesource.com/chromium/src.git/+/master/docs/memory/key_concepts.md , since it appeared to me that we free the memory from JS, but v8/chromium does not release the memory back to the OS. AFAIK that is typical behavior for many garbage-collected runtimes. So we simply stopped our investigation since this behavior does not come from our code-base and the behavior was different across Electron versions (with the same vscode code).

This could be electronjs issue and not solely vscode and that could be why it’s out of scope for vscode.

A quick test could be to create a quick json viewer using electronjs and see if the memory leak still occurs.

Allocating and freeing memory on demand without restrains will lead to memory fragmentation, degrading performance.

Need 100MB? Allocate it. Don’t need anymore? Ok, keep that space, we might need it soon.

VSCode maintainers were totally right to descope this “bug”. If you desperately want that memory back, I’d recommend checking with Node/V8 devs for JS APIs to control memory management and/or garbage collection.

Didn’t test this myself but holding memory is not necessary a “memory leak”. When we talk about memory allocations like malloc and free they don’t “delete” the memory and return it to the OS. What they do is usually mark is as available and if the OS needs it, it can take it back.

A better memory leak would be opening and closing the same file. If you see the memory starts jumping after a couple of tries there is a memory if not it reusing the freed space.

Edit: Sorry for duplicate explanation, so many people added a comment while I was typing 😅

To everyone who wants to comment please try to stick to actual constructive commentary and let’s try to avoid writing unnecessary comments as that’s hard to sift through to see what’s actually helping. They specifically wrote (well, bot answer):

If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

So far there was not even a reason given for why the team thinks it’s out-of-scope which might help us understand it more, maybe @bpasero could give us a short update on that one.

In general I’m agreeing with the previous commenters that performance should always be one of the priorities especially if you’re leaking like an old man. That goes to say that you can only do so much about that if that is actually in your codebase and not in some third parties.

It might help if we as a community try to isolate this behaviour and come up with statistics so if you want to actively help and try to get this solved I’d vouch for going into that direction.

I found this interesting – https://chromium.googlesource.com/chromium/src.git/+/master/docs/memory/key_concepts.md

I can get this to happen with files in the current workspace and outside of the current workspace. Opening the same file, closing it, and opening it again will cause an increase in memory usage proportional to the size of the file. The memory does not get reclaimed. This continues unbounded for me.

I can get the memory to be reclaimed by opening a new workspace. I’d argue that it makes it a memory leak, since I’m testing with the same file within a single workspace, then swapping to a new workspace and immediately getting reclamation of space. Maybe that’s expected behavior, but it’s a bad user experience anyway if I have to keep all of my files opened all the time to avoid leaks. So far, I’ve only found switching workspaces to reset memory usage without restart. Reloading the current workspace does not work.

Environment: Version: 1.51.1 (system setup) Commit: e5a624b788d92b8d34d1392e4c4d9789406efe8f Date: 2020-11-10T23:34:32.027Z Electron: 9.3.3 Chrome: 83.0.4103.122 Node.js: 12.14.1 V8: 8.3.110.13-electron.0 OS: Windows_NT x64 10.0.18363

thanks,I’ll use webstorm instead

@dataf3l said it best, if you don’t like the product, use another or pay for one where you can demand fixes… I understand why so many OS maintainers get sick of maintaining the codebases.

I’d like to point out, that since last time I’ve tested it (some months ago), it seemed like a regression because both leaking and non-leaking release of vscode were using the same electron version. However, I am sorry I haven’t taken notes and can’t really remember, nor do I have the resources and time to bisect this “regression”. The least I can say is that I can still reliably reproduce this memory leak.

/verified

vscode is being used by many online IDE services like gitpod. Such memory issues could impact their performance.

I believe I ran into this about a year ago (https://github.com/microsoft/vscode/issues/76498)

I have also been experiencing this problem, however I am not exactly working with large files but simple 10MB plain log files. The memory usage doesn’t drop after having several files opened and closed.

@bpasero Why did you mark this issue as “out-of-scope”. This is clearly an issue, except memory leaks are considering okay by the vscode team.

I’d also point out that this didn’t happen with earlier releases of vscode.

VSCode maintainers were totally right to descope this “bug”. If you desperately want that memory back, I’d recommend checking with Node/V8 devs for JS APIs to control memory management and/or garbage collection.

Maybe nudge the VSCode team to embed a CLR in VSCode as an alternative to JavaScript.