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.

Sign up for GitHub

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

Improve control flow type caching #35332

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Improve control flow type caching #35332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
725aa48
Cache getTypeOfExpression results when CFA was invoked
ahejlsberg Nov 25, 2019
5de5e09
Add tests
ahejlsberg Nov 25, 2019
2d19e83
Accept new baselines
ahejlsberg Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 39 additions & 24 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,7 @@ namespace ts {
let flowInvocationCount = 0;
let lastFlowNode: FlowNode | undefined;
let lastFlowNodeReachable: boolean;
let flowTypeCache: Type[] | undefined;

const emptyStringType = getLiteralType("");
const zeroType = getLiteralType(0);
Expand All @@ -844,7 +845,6 @@ namespace ts {
const symbolLinks: SymbolLinks[] = [];
const nodeLinks: NodeLinks[] = [];
const flowLoopCaches: Map<Type>[] = [];
const flowAssignmentTypes: Type[] = [];
const flowLoopNodes: FlowNode[] = [];
const flowLoopKeys: string[] = [];
const flowLoopTypes: Type[][] = [];
Expand Down Expand Up @@ -19173,23 +19173,9 @@ namespace ts {

function getInitialOrAssignedType(flow: FlowAssignment) {
const node = flow.node;
if (flow.flags & FlowFlags.Cached) {
const cached = flowAssignmentTypes[getNodeId(node)];
if (cached) {
return cached;
}
}
const startInvocationCount = flowInvocationCount;
const type = getConstraintForLocation(node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement ?
return getConstraintForLocation(node.kind === SyntaxKind.VariableDeclaration || node.kind === SyntaxKind.BindingElement ?
getInitialType(<VariableDeclaration | BindingElement>node) :
getAssignedType(node), reference);
// We cache the assigned type when getFlowTypeOfReference was recursively invoked in the
// resolution of the assigned type and we're not within loop analysis.
if (flowInvocationCount !== startInvocationCount && flowLoopCount === flowLoopStart) {
flow.flags |= FlowFlags.Cached;
flowAssignmentTypes[getNodeId(node)] = type;
}
return type;
}

function getTypeAtFlowAssignment(flow: FlowAssignment) {
Expand Down Expand Up @@ -19469,7 +19455,10 @@ namespace ts {
flowLoopKeys[flowLoopCount] = key;
flowLoopTypes[flowLoopCount] = antecedentTypes;
flowLoopCount++;
const saveFlowTypeCache = flowTypeCache;
flowTypeCache = undefined;
flowType = getTypeAtFlowNode(antecedent);
flowTypeCache = saveFlowTypeCache;
flowLoopCount--;
// If we see a value appear in the cache it is a sign that control flow analysis
// was restarted and completed by checkExpressionCached. We can simply pick up
Expand Down Expand Up @@ -27318,8 +27307,11 @@ namespace ts {
// analysis because variables may have transient types in indeterminable states. Moving flowLoopStart
// to the top of the stack ensures all transient types are computed from a known point.
const saveFlowLoopStart = flowLoopStart;
const saveFlowTypeCache = flowTypeCache;
flowLoopStart = flowLoopCount;
flowTypeCache = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Since we do it in two places, can we capture this "execute in new flow cache context" pattern into it's own helper function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is fine as is.

links.resolvedType = checkExpression(node, checkMode);
flowTypeCache = saveFlowTypeCache;
flowLoopStart = saveFlowLoopStart;
}
return links.resolvedType;
Expand All @@ -27332,7 +27324,7 @@ namespace ts {

function checkDeclarationInitializer(declaration: HasExpressionInitializer) {
const initializer = getEffectiveInitializer(declaration)!;
const type = getTypeOfExpression(initializer, /*cache*/ true);
const type = getQuickTypeOfExpression(initializer) || checkExpressionCached(initializer);
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, using the cache flag here was due to a perf issue, not a correctness one, and the caching was a correctness trade-off - using the per-loop cache is probably better, if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we need to call checkExpressionCached here because it resets the flow loop state to ensure everything is computed from scratch before the type is permanently cached. The getTypeOfExpression cache is purely there to help performance, but there is no guarantee that the same type will be returned every time.

const padded = isParameter(declaration) && declaration.name.kind === SyntaxKind.ArrayBindingPattern &&
isTupleType(type) && !type.target.hasRestElement && getTypeReferenceArity(type) < declaration.name.elements.length ?
padTupleType(type, declaration.name) : type;
Expand Down Expand Up @@ -27586,10 +27578,32 @@ namespace ts {
/**
* Returns the type of an expression. Unlike checkExpression, this function is simply concerned
* with computing the type and may not fully check all contained sub-expressions for errors.
* A cache argument of true indicates that if the function performs a full type check, it is ok
* to cache the result.
*/
function getTypeOfExpression(node: Expression, cache?: boolean) {
function getTypeOfExpression(node: Expression) {
Copy link
Member

Choose a reason for hiding this comment

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

We should document that we probably only expect this to be called into from flow analysis now - using it elsewhere is probably a mistake. I know we have a call into it from the node builder and services layers, too, but both of those should probably just be using checkExpressionCached.

// Don't bother caching types that require no flow analysis and are quick to compute.
const quickType = getQuickTypeOfExpression(node);
if (quickType) {
return quickType;
}
// If a type has been cached for the node, return it.
if (node.flags & NodeFlags.TypeCached && flowTypeCache) {
const cachedType = flowTypeCache[getNodeId(node)];
if (cachedType) {
return cachedType;
}
}
const startInvocationCount = flowInvocationCount;
const type = checkExpression(node);
// If control flow analysis was required to determine the type, it is worth caching.
if (flowInvocationCount !== startInvocationCount) {
const cache = flowTypeCache || (flowTypeCache = []);
cache[getNodeId(node)] = type;
node.flags |= NodeFlags.TypeCached;
}
return type;
}

function getQuickTypeOfExpression(node: Expression) {
const expr = skipParentheses(node);
// Optimize for the common case of a call to a function with a single non-generic call
// signature where we can just fetch the return type without checking the arguments.
Expand All @@ -27603,10 +27617,11 @@ namespace ts {
else if (isAssertionExpression(expr) && !isConstTypeReference(expr.type)) {
return getTypeFromTypeNode((<TypeAssertion>expr).type);
}
// Otherwise simply call checkExpression. Ideally, the entire family of checkXXX functions
// should have a parameter that indicates whether full error checking is required such that
// we can perform the optimizations locally.
return cache ? checkExpressionCached(node) : checkExpression(node);
else if (node.kind === SyntaxKind.NumericLiteral || node.kind === SyntaxKind.StringLiteral ||
node.kind === SyntaxKind.TrueKeyword || node.kind === SyntaxKind.FalseKeyword) {
return checkExpression(node);
}
return undefined;
}

/**
Expand Down
3 changes: 1 addition & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ namespace ts {
/* @internal */ Ambient = 1 << 23, // If node was inside an ambient context -- a declaration file, or inside something with the `declare` modifier.
/* @internal */ InWithStatement = 1 << 24, // If any ancestor of node was the `statement` of a WithStatement (not the `expression`)
JsonFile = 1 << 25, // If node was parsed in a Json
/* @internal */ TypeCached = 1 << 26, // If a type was cached for node at any point

BlockScoped = Let | Const,

Expand Down Expand Up @@ -2699,8 +2700,6 @@ namespace ts {
Shared = 1 << 11, // Referenced as antecedent more than once
PreFinally = 1 << 12, // Injected edge that links pre-finally label and pre-try flow
AfterFinally = 1 << 13, // Injected edge that links post-finally flow with the rest of the graph
/** @internal */
Cached = 1 << 14, // Indicates that at least one cross-call cache entry exists for this node, even if not a loop participant

Label = BranchLabel | LoopLabel,
Condition = TrueCondition | FalseCondition,
Expand Down
Loading