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

Instantiate anonymous types less #7138

Closed
wants to merge 8 commits into from

Conversation

sandersn
Copy link
Member

Fixes issue: #7097

This operates during instantiateType to skip instantiation of anonymous types whose declarations are not bound by the type mapper that will do the instantiation.

I'm not sure this is the right approach, but I want to get suggestions on the right way to do it so I thought a strawman would help the discussion.

This operates during `instantiateType` to skip instantiation of anonymous
types whose declarations are *not* bound by the type mapper that will do the
instantation.

Unfortunately, it fails for this types and union types, perhaps because
these do not have symbols. This types also don't have explicit type
parameters, so that may be the reason.

For example, for-of30 currently fails.
Each of the methods has a `typeof Class` argument which currently causes
an out-of-memory error when compiling a lot of these methods.
Since they have no static side.
Including aliases of intersection types.
@@ -4954,11 +4958,15 @@ namespace ts {
}

function createUnaryTypeMapper(source: Type, target: Type): TypeMapper {
return t => t === source ? target : t;
const mapper = <TypeMapper>(t => t === source ? target : t);
mapper.mapsType = sym => sym.name === source.symbol.name;
Copy link
Member

Choose a reason for hiding this comment

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

Is this like some sort of optimization?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the information that mappers previously did not expose -- which types they map. This is important because hasTypeParametersInScope will now tell its caller whether a type needs to be instantiated by a particular mapper.

This is only partly an optimization, because some of the types for which we now skip instantiation should never be instantiated, like the anonymous typeof ListWrapper types in the generic static functions in the test.

1. Use types directly instead of type's symbol's names. Comparing names was
incorrect and bluebird's .d.ts from DefinitelyTyped still hit the bug.
2. Use this-type of classes directly to see whether this-types are mapped.
This is more complicated than the angular example in that it has a lot of
generic statics inside a class that is itself generic. The class and
functions all use the same name for their type parameters.
@sandersn
Copy link
Member Author

sandersn commented Mar 7, 2016

Weird. The linter failed on protocol.d.t.s, which this PR doesn't touch. But only on the push CI build, not the PR one.

@DanielRosenwasser
Copy link
Member

It's possible you'll need to merge in from master to sync up with some unrelated changes.

@DanielRosenwasser
Copy link
Member

Are there any specific speed changes you see when compiling tsc with the new version?

@sandersn
Copy link
Member Author

sandersn commented Mar 8, 2016

I haven't looked, except to test on all the RWC projects -- angular2 and bluebird.d.ts both ran out of memory before and now run just fine. But there should be plenty of unneeded instantiations that are no longer happening. I'll try to run some perf tests later.

@ahejlsberg
Copy link
Member

@sandersn Just merged #7448 so I think we can close this one.

@mhegazy mhegazy closed this Mar 9, 2016
@mhegazy mhegazy deleted the instantiate-anonymous-types-less branch March 9, 2016 20:45
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

5 participants