Add upper limit for the program size to prevent tsserver from crashing #7486

zhengbli · 2016-03-11T23:59:28Z

Fixes #7444
For experimental purpose, the upper limit was set to 35 Mb (as suggested in #7444 (comment)), which might be changed to a better value after further tests.

Per our discussion in #7444, the next step is to add a compiler switch (something like disableSizeLimit) to disable this check for a project. Though I will do that after the current size-checking logic was reviewed.

Edit: also fix #8212

…nk files

billti · 2016-03-13T16:37:51Z

src/compiler/diagnosticMessages.json

@@ -2824,5 +2824,9 @@
    "Unknown typing option '{0}'.": {
        "category": "Error",
        "code": 17010
+    },
+    "Too many javascript files in the project. Consider add to the `exclude` list in the config file.": {


Grammar: "Consider adding to...". Or perhaps just reworded as "Use the 'exclude' setting in project configuration to limit included source folders".

Note: Do we predicate this message on the user not having listed the files to compile? If we are not scanning directories anyway (i.e. there is a list of files given), then this message would be misleading (as exclude can't be specified - at least until the globbing change comes in).

Also, capitalize as "JavaScript", not "javascript", and don't use backticks. This isn't markdown :-p

I agree that we should mention both cases, with or without the "exclude" list. Does the following sound better:

"Too many JavaScript files in the project. Use an exact 'files' list, or use the 'exclude' setting in project configuration to limit included source folders."

I like it :-)

billti · 2016-03-13T18:04:46Z

Added some comments. Note this pull request should be against release-1.8 also if VSCode wants it for their release, not against master. Thanks!

zhengbli · 2016-03-14T21:58:12Z

src/compiler/utilities.ts

@@ -2866,4 +2870,6 @@ namespace ts {
    export function isParameterPropertyDeclaration(node: ParameterDeclaration): boolean {
        return node.flags & NodeFlags.AccessibilityModifier && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent);
    }
+
+    export const maxProgramSize = 20 * 1024 * 1024;


Changed the upper limit from 35M to 20M, as on my machine even 35M would still run out of memory

mhegazy · 2016-03-14T22:31:14Z

src/compiler/commandLineParser.ts

+        {
+            name: "disableSizeLimit",
+            type: "boolean",
+            description: Diagnostics.Disable_the_upper_limit_for_the_total_file_size_of_a_project


remove the description, so that it does not show in the help.

zhengbli · 2016-03-15T03:30:31Z

@billti @mhegazy @vladima Please review the updated code, I'll try to merge into 1.8 once this PR got thumbs up. Thanks!

mhegazy · 2016-03-15T04:28:26Z

src/compiler/program.ts

@@ -1452,6 +1454,27 @@ namespace ts {
                return file;
            }

+            if (!options.disableSizeLimit) {
+                if (programSizeForNonTsFiles === -1) {
+                    return;


nit. return undefined;

Port #7486 to release 1.8

… fixLargeProjectTry2 # Conflicts: # src/compiler/types.ts

… fixLargeProjectTry2

zhengbli · 2016-03-18T17:03:34Z

With the new updates, the response time for the first "ProjectInfo" request was reduced to around 5s from 40+s for the repro sample at #7444 originally, due to avoiding creating programs for the project and speeding up in the file traversing process.

billti · 2016-03-18T17:36:55Z

src/compiler/commandLineParser.ts

                // Get files of supported extensions in their order of resolution
                for (const extension of supportedExtensions) {
-                    const filesInDirWithExtension = host.readDirectory(basePath, extension, exclude);


We were re-reading the directory for every file extension in the list? Crazy!! Good find 😄

billti · 2016-03-18T17:38:14Z

Looks good at first read. We should add the same logic on the VS host side also to improve Salsa experience there.

zhengbli · 2016-03-21T20:08:39Z

Ping @mhegazy

… fixLargeProjectTry2 # Conflicts: # src/compiler/commandLineParser.ts # src/compiler/types.ts

… fixLargeProjectTry2 # Conflicts: # src/compiler/sys.ts

mhegazy · 2016-06-09T19:53:53Z

src/compiler/program.ts

@@ -1463,6 +1477,25 @@ namespace ts {
                }
            });

+            if (!options.disableSizeLimit && file && file.text && !hasTypeScriptFileExtension(file.fileName)) {


looks like we do not need this in the program building, since we are doing it in the server.

… fixLargeProjectTry2 # Conflicts: # src/compiler/commandLineParser.ts # src/compiler/program.ts # src/compiler/sys.ts # src/compiler/types.ts # src/server/editorServices.ts # src/server/session.ts

mhegazy · 2016-06-09T23:41:32Z

src/compiler/commandLineParser.ts

@@ -723,10 +727,22 @@ namespace ts {
                const supportedExtensions = getSupportedExtensions(options);
                Debug.assert(indexOf(supportedExtensions, ".ts") < indexOf(supportedExtensions, ".d.ts"), "Changed priority of extensions to pick");

+                const potentialFiles: string[] = [];
+                if (host.readDirectoryWithMultipleExtensions) {
+                    addRange(potentialFiles, host.readDirectoryWithMultipleExtensions(basePath, supportedExtensions, exclude));


I believe this already handled by @riknoll in #8841. can we make sure we are doing one approach for both PRs.

possibly split this in a separate PR.

mhegazy · 2016-06-09T23:44:57Z

We need a unit test for this, now that we have unit tests :)

mhegazy · 2016-06-09T23:45:04Z

@vladima any comments?

zhengbli · 2016-06-10T09:08:02Z

@mhegazy updated with a test

mhegazy · 2016-06-15T21:18:58Z

src/server/editorServices.ts

@@ -1299,7 +1416,43 @@ namespace ts.server {
                    return errors;
                }
                else {
-                    const oldFileNames = project.compilerService.host.roots.map(info => info.fileName);
+                    if (this.exceedTotalNonTsFileSizeLimit(projectOptions.files) && projectOptions.compilerOptions && !projectOptions.compilerOptions.disableSizeLimit) {


move this !projectOptions.compilerOptions.disableSizeLimit to the beginning.

mhegazy · 2016-06-15T21:21:46Z

👍

… fixLargeProjectTry2 # Conflicts: # src/compiler/program.ts # tests/cases/unittests/tsserverProjectSystem.ts

Add upper limit for the program size, fix readDirectory for the symli…

4d3cff1

…nk files

msftclas added the cla-not-required label Mar 11, 2016

zhengbli mentioned this pull request Mar 12, 2016

tsserver crashes with large projects #7444

Closed

Add comments

b155fa8

billti reviewed Mar 13, 2016
View reviewed changes

CR feedback / Change upper limit / Add disableSizeLimit compiler option

a3aa000

zhengbli reviewed Mar 14, 2016
View reviewed changes

zhengbli added a commit to zhengbli/TypeScript that referenced this pull request Mar 14, 2016

Port microsoft#7486 to release-1.8

f22d14e

mhegazy reviewed Mar 14, 2016
View reviewed changes

zhengbli mentioned this pull request Mar 14, 2016

Port #7486 to release-1.8 #7508

Closed

online and offline CR feedback

a6a466c

Don't count current opened client file if it's TS file

d4eb3b8

zhengbli added a commit to zhengbli/TypeScript that referenced this pull request Mar 15, 2016

Port microsoft#7486 to release-1.8

5679991

mhegazy reviewed Mar 15, 2016
View reviewed changes

zhengbli added a commit that referenced this pull request Mar 15, 2016

Merge pull request #7513 from zhengbli/projectSizeLimitFor18-2

b23503c

Port #7486 to release 1.8

zhengbli added 2 commits March 16, 2016 20:53

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

a839d93

… fixLargeProjectTry2 # Conflicts: # src/compiler/types.ts

Speed up file searching

225e3b4

zhengbli added 2 commits March 17, 2016 17:11

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

d7e1d34

… fixLargeProjectTry2

Fix failed tests

cb46f16

zhengbli force-pushed the fixLargeProjectTry2 branch from 36b8172 to cb46f16 Compare March 18, 2016 02:58

billti reviewed Mar 18, 2016
View reviewed changes

Fix project updateing issue after editing config file

74e3d7b

zhengbli added 2 commits March 24, 2016 11:42

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

1b76294

… fixLargeProjectTry2 # Conflicts: # src/compiler/commandLineParser.ts # src/compiler/types.ts

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

5c9ce9e

… fixLargeProjectTry2 # Conflicts: # src/compiler/sys.ts

zhengbli mentioned this pull request Apr 20, 2016

JS error highlighting stops working in some cases #8212

Closed

mhegazy reviewed Jun 9, 2016
View reviewed changes

zhengbli added 3 commits June 9, 2016 14:58

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

94d44ad

… fixLargeProjectTry2 # Conflicts: # src/compiler/commandLineParser.ts # src/compiler/program.ts # src/compiler/sys.ts # src/compiler/types.ts # src/server/editorServices.ts # src/server/session.ts

Fix merging issues and multiple project scenario

d387050

Refactoring

4383f1a

mhegazy reviewed Jun 9, 2016
View reviewed changes

add test and spit commandLineParser changes to another PR

e41b10b

zhengbli force-pushed the fixLargeProjectTry2 branch from c0e232e to e41b10b Compare June 10, 2016 09:06

mhegazy reviewed Jun 15, 2016
View reviewed changes

zhengbli added 2 commits June 15, 2016 14:36

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

3354436

… fixLargeProjectTry2 # Conflicts: # src/compiler/program.ts # tests/cases/unittests/tsserverProjectSystem.ts

Refactor code to make if statements cheaper

550d912

zhengbli force-pushed the fixLargeProjectTry2 branch from c6f9e25 to 550d912 Compare June 15, 2016 23:52

zhengbli merged commit 95ae809 into microsoft:master Jun 16, 2016

zhengbli deleted the fixLargeProjectTry2 branch June 16, 2016 00:31

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upper limit for the program size to prevent tsserver from crashing #7486

Add upper limit for the program size to prevent tsserver from crashing #7486

zhengbli commented Mar 11, 2016 •

edited

Loading

billti Mar 13, 2016

zhengbli Mar 14, 2016

billti Mar 14, 2016

billti commented Mar 13, 2016

zhengbli Mar 14, 2016

mhegazy Mar 14, 2016

zhengbli commented Mar 15, 2016

mhegazy Mar 15, 2016

zhengbli commented Mar 18, 2016

billti Mar 18, 2016

billti commented Mar 18, 2016

zhengbli commented Mar 21, 2016

mhegazy Jun 9, 2016

mhegazy Jun 9, 2016

mhegazy Jun 9, 2016

zhengbli Jun 10, 2016

mhegazy commented Jun 9, 2016

mhegazy commented Jun 9, 2016

zhengbli commented Jun 10, 2016

mhegazy Jun 15, 2016

mhegazy commented Jun 15, 2016

Add upper limit for the program size to prevent tsserver from crashing #7486

Add upper limit for the program size to prevent tsserver from crashing #7486

Conversation

zhengbli commented Mar 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billti commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Mar 15, 2016

Choose a reason for hiding this comment

zhengbli commented Mar 18, 2016

Choose a reason for hiding this comment

billti commented Mar 18, 2016

zhengbli commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Jun 9, 2016

mhegazy commented Jun 9, 2016

zhengbli commented Jun 10, 2016

Choose a reason for hiding this comment

mhegazy commented Jun 15, 2016

zhengbli commented Mar 11, 2016 •

edited

Loading