Skip to content

Fixes the memory leak because of project and its corresponding script info even after project is removed#16494

Merged
sheetalkamat merged 4 commits into
masterfrom
dontCreateScriptInfosWithFileExistsAndReadFile
Jun 14, 2017
Merged

Fixes the memory leak because of project and its corresponding script info even after project is removed#16494
sheetalkamat merged 4 commits into
masterfrom
dontCreateScriptInfosWithFileExistsAndReadFile

Conversation

@sheetalkamat
Copy link
Copy Markdown
Member

@sheetalkamat sheetalkamat commented Jun 13, 2017

  • fileExists and readFile of the CompilerHost created by Language service created script infos but it shouldn't because the getSourceFile is the correct api to get those since the script infos (scriptsnapshot and watcher for the file) are always associated to the project they are requested from. The issue here was that the script infos created by fileExists and readFile would get the project assigned but never removed because they weren't part of project's source file. 1bf1209 fixes this by directing those call to host instead.
  • When project is removed it removed the script shot of open file/s from that project but it never use to remove it for files are not open. For big projects that meant huge strings and rest of the stuff still in memory. 2ec92b9 cleans up the ScriptInfos not associated with any project to reduce footprint left behind by closing files. This change also cleans up few pointers to make sure project, LShost, LS and other structures that are related to the project are cleaned up
  • 98cb0ce moves the cleanup of orphan script infos to after next open file so that we can reuse the script infos if the file from the previously closed project is opened right after that.

Fixes microsoft/vscode#28112 and #16329

…t is closed or inferred projects are refreshed

Also dispose some pointers so that the closures get disposed with project and script infos
Comment thread src/server/editorServices.ts Outdated
}

// Cleanup script infos that are not open and not part of any project
this.deleteOrphanScriptInfoNotInAnyProject();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I though we will do this in openClientFile?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was before discussing that.. I am working on moving this to openClientFile

@nishp1
Copy link
Copy Markdown

nishp1 commented Jun 14, 2017

Thanks @sheetalkamat for taking this on. Is there anyway I can help verify this bug against Code v1.13 or insider build?

This helps in reusing script infos even if the project is closed but next open recreates the same project
// at this point if file is the part of some configured/external project then this project should be created
const info = this.getOrCreateScriptInfoForNormalizedPath(fileName, /*openedByClient*/ true, fileContent, scriptKind, hasMixedContent);
this.assignScriptInfoToInferredProjectIfNecessary(info, /*addToListOfOpenFiles*/ true);
this.deleteOrphanScriptInfoNotInAnyProject();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please add a comment here on why we do this on open, just to make sure ppl do not wonder in the future why this is here and not in close project.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure

@sheetalkamat sheetalkamat merged commit 187febd into master Jun 14, 2017
@sheetalkamat sheetalkamat deleted the dontCreateScriptInfosWithFileExistsAndReadFile branch June 14, 2017 22:17
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jun 14, 2017

we need this in release-2.4 as well.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeScript server crashes faster than ever

4 participants