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

Printing JS constructor function type causes out-of-memory error #29597

Closed
bowenni opened this issue Jan 26, 2019 · 20 comments · Fixed by #31586 or #32381
Closed

Printing JS constructor function type causes out-of-memory error #29597

bowenni opened this issue Jan 26, 2019 · 20 comments · Fixed by #31586 or #32381
Assignees
Labels
Bug A bug in TypeScript Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output Fixed A PR has been merged for this issue

Comments

@bowenni
Copy link

bowenni commented Jan 26, 2019

TypeScript Version: 3.2.2 and 3.3.0-rc

Search Terms:
heap out of memory

Code
First of all, I don't expect there to be a valid use case of what I'm describing below. In fact a miss configuration in my build settings lead me to this. However I don't expect TSC to completely crash. Also I found that TS 3.1 had no problems with the code below so this might be a regression of TS 3.2.

Steps to reproduce the heap OOM

  • npm install chart.js
  • cd into node_modules/chart.js/dist. You should find Chart.min.js there.
  • create a tsconfig.json with the following content
{
  "compilerOptions": {
      "target": "es5",
      "module": "commonjs",
      "downlevelIteration": true,
      "skipDefaultLibCheck": true,
      "moduleResolution": "node",
      "preserveConstEnums": false,
      "experimentalDecorators": true,
      "emitDecoratorMetadata": true,
      "jsx": "react",
      "noErrorTruncation": true,
      "noEmitOnError": false,
      "declaration": false,
      "stripInternal": true,
      "inlineSourceMap": true,
      "inlineSources": true,
      "sourceMap": false,
      "jsxFactory": "react",
      "noImplicitAny": true,
      "noImplicitReturns": true,
      "noImplicitThis": true,
      "noFallthroughCasesInSwitch": true,
      "strictNullChecks": true,
      "strictPropertyInitialization": true,
      "strictFunctionTypes": true,
      "noStrictGenericChecks": true,
      "noResolve": true,
      "types": [],
      "importHelpers": false,
      "allowJs": true,
      "checkJs": false
  },
  "files": [
      "./Chart.min.js",
  ],
  "compileOnSave": false
}
  • run tsc -p tsconfig.json --outDir foo and see the JavaScript heap out of memory thrown.

Expected behavior:
Emits a file foo/Chart.min.js.
TS 3.1.6 finishes this in 1.7 seconds without any errors.

Actual behavior:
FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Playground Link:
N/A

Related Issues:
This looks similar to #29326 and #29511 but I don't understand it deep enough to tell if it's a duplicate.

@fatcerberus
Copy link

Wow, it actually runs V8 out of heap space, that's impressive 😮

@bowenni
Copy link
Author

bowenni commented Jan 29, 2019

I created a smaller repro in https://github.com/bowenni/heap-OOM-repro

The relevant compiler options are
"noErrorTruncation": true
"noImplicitAny": true
"allowJs": true

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Feb 4, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.4.0 milestone Feb 4, 2019
@RyanCavanaugh RyanCavanaugh added the Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output label Feb 4, 2019
@sandersn
Copy link
Member

Still crashes on 3.4 and typescript@next

@sandersn
Copy link
Member

Also crashes with the unminified chart.js

@sandersn
Copy link
Member

After shrinking the size of Chart.js considerably, I can see that the problem is that Color is a large class defined by prototype assignment whose methods return this. Something like this:

var Color = function (obj) {
    this.values = { rgb: [1, 1, 1] }
};

Color.prototype = {
// .........
	negate: function () {
		var rgb = [];
		for (var i = 0; i < 3; i++) {
			rgb[i] = 255 - this.values.rgb[i];
		}
		this.setValues('rgb', rgb);
		return this;
	},

	lighten: function (ratio) {
// ....

When you ask for the type of this in return this, we keep re-evaluating the type for this for each methods that returns this. The class type is not used since there is no class type created for constructor functions. Instead the class type is re-created over and over again, resulting in a factorial type structure:

this: {
  negate: () => {
    negate: any
    lighten: (ratio: any) => {
      negate: any
      lighten: any
      darken: (ratio: any) => {
        negate: any
        lighten: any
        darken: any
        greyscale: (ratio: any) => {
          negate: any
          lighten: any
          darken: any
          greyscale: any
          blacken: ...

The root cause is that constructor functions don't create classes. This type shouldn't be created over and over again, but a better fix is to make constructor functions create real classes.

@sandersn sandersn changed the title JS compilation causes heap OOM Trying to print JS constructor function type causes out-of-memory error May 17, 2019
@sandersn sandersn changed the title Trying to print JS constructor function type causes out-of-memory error Printing JS constructor function type causes out-of-memory error May 17, 2019
@sandersn sandersn modified the milestones: TypeScript 3.5.0, Backlog May 17, 2019
@dgreene1
Copy link

Is there a workaround for this in the meantime?

It's unclear on how one should proceed when they get this error.

@bowenni
Copy link
Author

bowenni commented May 28, 2019

@dgreene1 I worked around the problem by setting noErrorTruncation to false.

@dgreene1
Copy link

That workaround (adding "noErrorTruncation": false) didn't eliminate the problem for me.

@dgreene1
Copy link

dgreene1 commented May 28, 2019

For reasons I don't think I'll ever understand, this is the line that was causing the issue:

				return _.chain(res.data.data)
					.get('attributes.objects', [])
					.tap(allUserPermissionObjects => {
						log.debug(`allUserPermissionObjects for userId(${userId}): `, allUserPermissionObjects.length);
					})
					.find(po => po.name === 'DataImport')
					.get('permittedActions', [])
					.value();

Hopefully that helps someone to debug the root issue, which I'm sure has nothing to do with lodash.

@sandersn
Copy link
Member

@dgreene1 You have a different issue if noErrorTruncation didn't work.

lodash chaining methods were quite inefficient to compile until recently. Can you update your @types/lodash dependency to see if that could be the cause?

@weswigham
Copy link
Member

weswigham commented May 28, 2019

@sandersn I think if you look at your trimmed down example (based on chart.min.js) again with what we have in master, you'll find a different underlying perf issue now - minimally, I tried to see if #31586 alone was enough to fix the OP's repro, and it was not, meaning that while that was probably occurring given the ouput, there's likely something else causing excessive resource usage, too.

@dgreene1
Copy link

lodash chaining methods were quite inefficient to compile until recently. Can you update your @types/lodash dependency to see if that could be the cause?

That didn't fix it. So I refactored the chaining code out and it's working.

@sandersn sandersn modified the milestones: Backlog, TypeScript 3.6.0 May 28, 2019
@sandersn
Copy link
Member

The trimmed-down example no longer crashes, but both Chart.js and chart.min.js still crash. I'll try to find the remaining problem(s).

@sandersn
Copy link
Member

sandersn commented Jul 12, 2019

Here's the trimmed down example that still crashes:

function Color(obj) {
    this.example = true
};
Color.prototype = {
	negate: function () {return this;},
	lighten: function (ratio) {return this;},
	darken: function (ratio) {return this;},
	saturate: function (ratio) {return this;},
	desaturate: function (ratio) {return this;},
	whiten: function (ratio) {return this;},
	blacken: function (ratio) {return this;},
	greyscale: function () {return this;},
	clearer: function (ratio) {return this;},
	toJSON: function () {return this.rgb();},
};

with this tsconfig:

{
  "compilerOptions": {
      "noErrorTruncation": true,
      "noImplicitAny": true,
      "allowJs": true,
      "outDir": "built"
  },
  "files": [
      "./Chart.js",
  ]
}

@weswigham
Copy link
Member

Is this.rbg being missing intentional?

@sandersn
Copy link
Member

Removing one function, or even the body of one function, allows compilation to finish in about 12 seconds.

@sandersn
Copy link
Member

@weswigham When changed to this.negate(), which does exist, it no longer crashes. Mysterious!

@sandersn
Copy link
Member

Oh, it's because we're trying to issue an error. If noErrorTruncation: false then there is no crash either.

@weswigham
Copy link
Member

Ah, alright

@sandersn
Copy link
Member

The root problem is typeToTypeNode doesn't expect recursive anonymous types, but the above example constructs an intersection of the type for function Color and Color.prototype, and the type of Color.prototype has methods that return this.

We can patch typeToTypeNodeHelper to understand this structure, or we can make constructor functions create class types. I'd rather do the latter, although it's a big task, so I'm going to look at my other 3.6 bugs first.

I'm not going to dupe this to the existing "make constructor functions into class types" bug because we might decide to solve this by adding a special case to typeToTypeNode. (#27328? #26883? There's not really a good issue for this anyway.)

@weswigham weswigham assigned weswigham and unassigned sandersn Jul 12, 2019
@weswigham weswigham added the Fixed A PR has been merged for this issue label Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Crash For flagging bugs which are compiler or service crashes or unclean exits, rather than bad output Fixed A PR has been merged for this issue
Projects
None yet
6 participants