GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add upper limit for the program size to prevent tsserver from crashing by zhengbli · Pull Request #7486 · microsoft/TypeScript

Conversation

Zhengbo Li (zhengbli) commented Mar 11, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

4d3cff1

…nk files

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

Mar 11, 2016

Zhengbo Li (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

Comment thread

src/compiler/diagnosticMessages.json Outdated

		"category": "Error",
		"code": 17010
		},
		"Too many javascript files in the project. Consider add to the `exclude` list in the config file.": {

Bill Ticehurst (billti) Mar 13, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Zhengbo Li (zhengbli) Mar 14, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Bill Ticehurst (billti) Mar 14, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I like it :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Bill Ticehurst (billti) commented Mar 13, 2016

Copy link

Copy Markdown

Member

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          CR feedback / Change upper limit / Add disableSizeLimit compiler option

a3aa000

zhengbli reviewed

Mar 14, 2016

View reviewed changes

Comment thread

src/compiler/utilities.ts Outdated

		return node.flags & NodeFlags.AccessibilityModifier && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent);
		}

		export const maxProgramSize = 20 * 1024 * 1024;

Zhengbo Li (zhengbli) Mar 14, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Zhengbo Li (zhengbli) pushed 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

Comment thread

src/compiler/commandLineParser.ts Outdated

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

Mohamed Hegazy (mhegazy) Mar 14, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Zhengbo Li (zhengbli) mentioned this pull request

Mar 14, 2016

Port #7486 to release-1.8 #7508

Closed


          online and offline CR feedback

a6a466c

Zhengbo Li (zhengbli) commented Mar 15, 2016

Copy link

Copy Markdown

Author

Bill Ticehurst (@billti) Mohamed Hegazy (@mhegazy) Vladimir Matveev (@vladima) Please review the updated code, I'll try to merge into 1.8 once this PR got thumbs up. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

d4eb3b8

Zhengbo Li (zhengbli) pushed a commit to zhengbli/TypeScript that referenced this pull request

Mar 15, 2016


          Port microsoft#7486 to release-1.8

mhegazy reviewed

Mar 15, 2016

View reviewed changes

Comment thread

src/compiler/program.ts Outdated


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

Mohamed Hegazy (mhegazy) Mar 15, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit. return undefined;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Zhengbo Li (zhengbli) pushed 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

Zhengbo Li (zhengbli) force-pushed the fixLargeProjectTry2 branch from 36b8172 to cb46f16 Compare

March 18, 2016 02:58

Zhengbo Li (zhengbli) commented Mar 18, 2016

Copy link

Copy Markdown

Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

billti reviewed

Mar 18, 2016

View reviewed changes

Comment thread

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);

Bill Ticehurst (billti) Mar 18, 2016

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Bill Ticehurst (billti) commented Mar 18, 2016

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix project updateing issue after editing config file

74e3d7b

Zhengbo Li (zhengbli) commented Mar 21, 2016

Copy link

Copy Markdown

Author

Ping Mohamed Hegazy (@mhegazy)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Zhengbo Li (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

Comment thread

src/compiler/program.ts Outdated

		}
		});

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

Mohamed Hegazy (mhegazy) Jun 9, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

src/compiler/commandLineParser.ts Outdated


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

Mohamed Hegazy (mhegazy) Jun 9, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Jun 9, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

possibly split this in a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Zhengbo Li (zhengbli) Jun 10, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) commented Jun 9, 2016

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mohamed Hegazy (mhegazy) commented Jun 9, 2016

Copy link

Copy Markdown

Contributor

Vladimir Matveev (@vladima) any comments?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          add test and spit commandLineParser changes to another PR

e41b10b

Zhengbo Li (zhengbli) force-pushed the fixLargeProjectTry2 branch from c0e232e to e41b10b Compare

June 10, 2016 09:06

Zhengbo Li (zhengbli) commented Jun 10, 2016

Copy link

Copy Markdown

Author

Mohamed Hegazy (@mhegazy) updated with a test

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhegazy reviewed

Jun 15, 2016

View reviewed changes

Comment thread

src/server/editorServices.ts Outdated

		}
		else {
		const oldFileNames = project.compilerService.host.roots.map(info => info.fileName);
		if (this.exceedTotalNonTsFileSizeLimit(projectOptions.files) && projectOptions.compilerOptions && !projectOptions.compilerOptions.disableSizeLimit) {

Mohamed Hegazy (mhegazy) Jun 15, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) commented Jun 15, 2016

Copy link

Copy Markdown

Contributor

👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zhengbli added 2 commits

June 15, 2016 14:36


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

… fixLargeProjectTry2

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


          Refactor code to make if statements cheaper

550d912

Zhengbo Li (zhengbli) force-pushed the fixLargeProjectTry2 branch from c6f9e25 to 550d912 Compare

June 15, 2016 23:52

Zhengbo Li (zhengbli) merged commit 95ae809 into microsoft:master

Jun 16, 2016

Zhengbo Li (zhengbli) deleted the fixLargeProjectTry2 branch

June 16, 2016 00:31

Microsoft (microsoft) locked and limited conversation to collaborators

Jun 19, 2018

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JS error highlighting stops working in some cases tsserver crashes with large projects

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Zhengbo Li (zhengbli) commented Mar 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bill Ticehurst (billti) commented Mar 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhengbo Li (zhengbli) commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zhengbo Li (zhengbli) commented Mar 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bill Ticehurst (billti) commented Mar 18, 2016

Uh oh!

Zhengbo Li (zhengbli) commented Mar 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Jun 9, 2016

Uh oh!

Mohamed Hegazy (mhegazy) commented Jun 9, 2016

Uh oh!

Zhengbo Li (zhengbli) commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Jun 15, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Zhengbo Li (zhengbli) commented Mar 11, 2016 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.