Improve caching of anonymous types#18231
Conversation
|
We should get some memory numbers for large or medium projects that don't benefit from this (if there are there are projects like this) to make sure there's not too much overhead. |
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
I have a couple of questions below.
| @@ -3469,8 +3468,6 @@ namespace ts { | |||
| /* @internal */ | |||
| export interface TypeMapper { | |||
There was a problem hiding this comment.
with this change, it would now be easier to read as type TypeMapper = (t: TypeParameter) => Type;
| if (!typeParameters) { | ||
| // The first time an anonymous type is instantiated we compute and store a list of the type | ||
| // parameters that are in scope (and therefore potentially referenced). For type literals that | ||
| // aren't the right hand side of a generic type alias declaration we optimize by reducing the |
There was a problem hiding this comment.
should be "the rhs of a non-generic type alias declaration", right?
There was a problem hiding this comment.
No, we optimize for type literals, except when they are the RHS of a generic type alias. For those we want to preserve the distinct instantiations even when they don't reference the type parameters (for fidelity when displaying types).
| return node.kind === SyntaxKind.ThisType || forEachChild(node, checkThis); | ||
| } | ||
| function checkIdentifier(node: Node): boolean { | ||
| return node.kind === SyntaxKind.Identifier && isPartOfTypeNode(node) && getTypeFromTypeNode(<TypeNode>node) === tp || forEachChild(node, checkIdentifier); |
There was a problem hiding this comment.
This call to getTypeFromTypeNode is likely to 'pull' a lot of types that might otherwise never be instantiated themselves. It's probably nothing because, during instantiation, we'll surely be instantiating nearby types anyway, but worth thinking about nonetheless.
There was a problem hiding this comment.
No, it is always fine to call getTypeFromTypeNode. It will be called by the checker anyways when as we type check a program.
| links.typeParameters = typeParameters; | ||
| if (typeParameters.length) { | ||
| links.instantiations = createMap<Type>(); | ||
| links.instantiations.set(getTypeListId(typeParameters), target); |
There was a problem hiding this comment.
I don't understand how caching the uninstantiated type helps.
There was a problem hiding this comment.
The uninstantiated type is the same as an instantiation where each type parameter has itself as its type argument. No point in recomputing that instantiation, we already have it.
| case SyntaxKind.TypeAliasDeclaration: | ||
| case SyntaxKind.JSDocTemplateTag: | ||
| case SyntaxKind.MappedType: | ||
| const outerTypeParameters = getOuterTypeParameters(node, includeThisTypes); |
There was a problem hiding this comment.
Why is this line needed? Aren't further iterations of the while loop equivalent to calling getOuterTypeParameters recursively?
There was a problem hiding this comment.
Oh, because we always return whenever any of these cases is true. Why not append to a local list, break, and keep looping instead of recurring and returning? Seems like it would be more efficient (as long as the final result isn't order-dependent, but that could be fixed by swapping the order in calls to append).
There was a problem hiding this comment.
Our use of the getOuterTypeParameters function in code for classes and interfaces requires the list to be in outside-in order. Hence the recursion. We could insert new elements at the beginning of the list or reverse the list at the end instead, but neither are better solutions.
|
@sandersn Regarding memory usage, it varies from no change to massive savings, but I haven't seen meaningfully increased consumption anywhere. There is no significant change for the Monaco and TFS code bases, for example. |
3 participants
We have recently seen a number of out-of-memory issues that are caused by runaway anonymous object type instantiations and comparisons. This PR introduces a new and much improved caching scheme for such types. The key idea is to track which type variables are in scope at the source origin of each anonymous object type and cache instantiations of the type based on the actual type arguments for those type variables. This means anonymous object type instantiation caching becomes as efficient as that for classes and interfaces, and out-of-memory issues that were previously fixed by switching from type aliases to interfaces should now be gone.
The RWC tests all pass. Only changes are some missing elaborations in error messages that result from improved sharing of identical types.
Fixes #16511.
Fixes #17036.
Fixes #17968.