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

Massive slowdown in compiling complicated union types #5849

Closed
Arnavion opened this issue Dec 1, 2015 · 18 comments
Closed

Massive slowdown in compiling complicated union types #5849

Arnavion opened this issue Dec 1, 2015 · 18 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@Arnavion
Copy link
Contributor

Arnavion commented Dec 1, 2015

This is the same code as what originally prompted #2997

While it isn't crashing because of infinite loops any more, it has become very slow with 1.7. If I pause it in the debugger I see the following stack repeating many times.

inferFromTypes [tsc.js] Line 15334  JavaScript
inferFromTypes [tsc.js] Line 15314  JavaScript
inferFromTypes [tsc.js] Line 15301  JavaScript
inferFromProperties [tsc.js] Line 15350 JavaScript
inferFromTypes [tsc.js] Line 15334  JavaScript
inferFromTypes [tsc.js] Line 15314  JavaScript
inferFromTypes [tsc.js] Line 15301  JavaScript
inferFromIndexTypes [tsc.js] Line 15380 JavaScript
inferFromTypes [tsc.js] Line 15337  JavaScript
inferFromProperties [tsc.js] Line 15350 JavaScript

Reduced test case:

class Module {
    public members: { [name: string]: FFunction | Property };
}

class Namespace {
    public members: { [name: string]: Class | Enum };
}

class Class {
    public parent: Module | Namespace;

    public members: { [name: string]: Property | FFunction };
}

class Interface {
    public members: { [name: string]: Property | FFunction };
}

class FFunction {
    public parent: Module | Namespace | Class | Interface; // (1)
}

class Property {
    public parent: Module | Namespace | Class | Interface; // (2)
}

class Enum {
    public parent: Module | Namespace;

    public members: EnumMember[];
}

class EnumMember {
    public parent: Enum;
}

var members: (Property | FFunction)[];
members.map(member => "");

Compile with tsc foo.ts on node 4.2.2 x64 on Windows 7.

This takes ~18s on my machine. If I remove Module from the union type at (1), it drops to ~12s. Removing Module at (2) drops it to ~6s. And so on.

The original code is at https://github.com/Arnavion/libjass/blob/82f3706/build/typescript/ast.ts which has a much more complicated forest of types. It's been compiling for 5 minutes now, whereas it took ~2s with 1.6

@vladima
Copy link
Contributor

vladima commented Dec 1, 2015

@Arnavion can you give a try to typescript@next. Seems that this issue is already fixed in master. I can confirm that it takes 17.50 s on my machine to compile your sample using release-1.7, however on master it takes only 0.71 s

@Arnavion
Copy link
Contributor Author

Arnavion commented Dec 1, 2015

Yes, looks like it. The original code also takes ~2s with next, same as 1.6.

@mhegazy mhegazy added Fixed A PR has been merged for this issue Bug A bug in TypeScript labels Dec 1, 2015
@mhegazy mhegazy added this to the TypeScript 1.8 milestone Dec 1, 2015
@mhegazy mhegazy closed this as completed Dec 1, 2015
@Arnavion
Copy link
Contributor Author

Arnavion commented Dec 1, 2015

Can the fix be backported to 1.7 or do I skip it and wait for 1.8?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

depends on the fix. @vladima do you know what is going on?

@vladima
Copy link
Contributor

vladima commented Dec 1, 2015

yes: #5738. If source and target types are unions or intersections then removing matching constituents from source and target sides significantly reduces amount of types that participate in type inference. I think it should be safe to backport this fix to 1.7

@mhegazy mhegazy reopened this Dec 1, 2015
@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label Dec 1, 2015
@Arnavion
Copy link
Contributor Author

Arnavion commented Dec 2, 2015

Yes, cherry-picking that PR onto 1.7 fixes the issue (and introduces #5861 but that's a minor annoyance in comparison).

@mhegazy
Copy link
Contributor

mhegazy commented Dec 2, 2015

@Arnavion is there a chance you can move to typescript@next directly?

@Arnavion
Copy link
Contributor Author

Arnavion commented Dec 2, 2015

If you're asking because you don't want to backport it, that's fine. I have no problem staying with 1.6 until 1.8 is released. :)

@mhegazy
Copy link
Contributor

mhegazy commented Dec 2, 2015

I am asking to figure out the logistics, porting it and publishing an npm is easy, but we will need to publish a VS 2012, VS 2015, and sublime text plugin updates and that all adds up.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 3, 2015

Let's port #5738 and #5895 to release-1.7 branch.

@ahejlsberg
Copy link
Member

I'm trying to understand why this got slower from 1.6 to 1.7? Vlad, did you look into that? I understand why the new union/intersection reduction in 1.8 makes it faster, but I'd like to know why it got slower in the first place.

@ahejlsberg
Copy link
Member

Ok, I now understand why it got slower. When inferring types, if the source and target are both non-generic object types we don't descend into the types because it we know it won't produce inferences. However, in 1.7 all classes are generic because of the implied this type argument, so we end up doing a lot more work. I'm going to give some thought to a fix. The union/intersection reduction isn't really the answer, it just so happens to help in this example.

@vladima
Copy link
Contributor

vladima commented Dec 4, 2015

yes, this got slower in the 9438a4b

@ahejlsberg
Copy link
Member

Ok, I have a fix. I will putting up a PR shortly.

@ahejlsberg
Copy link
Member

I would recommend not back-porting the union/intersection type reduction, but instead back-porting the #5920 fix I just put up.

@ahejlsberg ahejlsberg assigned ahejlsberg and unassigned vladima Dec 4, 2015
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Dec 4, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Dec 4, 2015

@Arnavion can you do us a big favor and give branch release-1.7 a quick test?

@Arnavion
Copy link
Contributor Author

Arnavion commented Dec 5, 2015

Everything seems fine. Thanks!

@mhegazy
Copy link
Contributor

mhegazy commented Dec 7, 2015

thanks @Arnavion!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants