From 3f198e6751892a531d20524300bba34544c02bf2 Mon Sep 17 00:00:00 2001 From: Jason Ramsay Date: Fri, 17 Feb 2017 13:54:07 -0800 Subject: [PATCH 1/5] Adding cancellation token checks for lower priority tasks --- .../unittests/tsserverProjectSystem.ts | 82 +++++++ src/services/navigationBar.ts | 232 +++++++++--------- src/services/outliningElementsCollector.ts | 4 +- src/services/services.ts | 6 +- 4 files changed, 207 insertions(+), 117 deletions(-) diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index 92ec0b4ee0..01e484fd4b 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -3460,6 +3460,88 @@ namespace ts.projectSystem { return JSON.parse(server.extractMessage(host.getOutput()[n])); } }); + it("Lower priority tasks are cancellable", () => { + const f1 = { + path: "/a/app.ts", + content: `{ let x = 1; } var foo = "foo"; var bar = "bar"; var fooBar = "fooBar";` + }; + const config = { + path: "/a/tsconfig.json", + content: JSON.stringify({ + compilerOptions: {} + }) + }; + + let requestToCancel = -1; + let isCancellationRequestedCount = 0; + let cancelAfterRequest = 3; + let operationCanceledExceptionThrown = false; + const cancellationToken: server.ServerCancellationToken = (function () { + let currentId: number; + return { + setRequest(requestId) { + currentId = requestId; + }, + resetRequest(requestId) { + assert.equal(requestId, currentId, "unexpected request id in cancellation") + currentId = undefined; + }, + isCancellationRequested() { + isCancellationRequestedCount++; + return requestToCancel === currentId && isCancellationRequestedCount >= cancelAfterRequest; + } + } + })(); + const host = createServerHost([f1, config]); + const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken); + { + session.executeCommandSeq({ + command: "open", + arguments: { file: f1.path } + }); + + // send navbar request (normal priority) + session.executeCommandSeq({ + command: "navbar", + arguments: { file: f1.path } + }); + + // ensure the nav bar request can be canceled + verifyExecuteCommandSeqIsCancellable({ + command: "navbar", + arguments: { file: f1.path } + }); + + // send outlining spans request (normal priority) + session.executeCommandSeq({ + command: "outliningSpans", + arguments: { file: f1.path } + }); + + // ensure the outlining spans request can be canceled + verifyExecuteCommandSeqIsCancellable({ + command: "outliningSpans", + arguments: { file: f1.path } + }); + } + + function verifyExecuteCommandSeqIsCancellable(request: Partial) { + // Set the next request to be cancellable + // The cancellation token will cancel the request the third time + // isCancellationRequested() is called. + requestToCancel = session.getNextSeq(); + isCancellationRequestedCount = 0; + operationCanceledExceptionThrown = false; + + try { + session.executeCommandSeq(request); + } catch (e) { + assert(e instanceof OperationCanceledException); + operationCanceledExceptionThrown = true; + } + assert(operationCanceledExceptionThrown); + } + }); }); describe("maxNodeModuleJsDepth for inferred projects", () => { diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 25059d1f57..37b6bedbd5 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -14,16 +14,16 @@ namespace ts.NavigationBar { indent: number; // # of parents } - export function getNavigationBarItems(sourceFile: SourceFile): NavigationBarItem[] { + export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarItem[] { curSourceFile = sourceFile; - const result = map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem); + const result = map(topLevelItems(rootNavigationBarNode(sourceFile, cancellationToken)), convertToTopLevelItem); curSourceFile = undefined; return result; } - export function getNavigationTree(sourceFile: SourceFile): NavigationTree { + export function getNavigationTree(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationTree { curSourceFile = sourceFile; - const result = convertToTree(rootNavigationBarNode(sourceFile)); + const result = convertToTree(rootNavigationBarNode(sourceFile, cancellationToken)); curSourceFile = undefined; return result; } @@ -55,12 +55,12 @@ namespace ts.NavigationBar { const parentsStack: NavigationBarNode[] = []; let parent: NavigationBarNode; - function rootNavigationBarNode(sourceFile: SourceFile): NavigationBarNode { + function rootNavigationBarNode(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarNode { Debug.assert(!parentsStack.length); const root: NavigationBarNode = { node: sourceFile, additionalNodes: undefined, parent: undefined, children: undefined, indent: 0 }; parent = root; for (const statement of sourceFile.statements) { - addChildrenRecursively(statement); + addChildrenRecursively(statement, cancellationToken); } endNode(); Debug.assert(!parent && !parentsStack.length); @@ -103,138 +103,144 @@ namespace ts.NavigationBar { parent = parentsStack.pop(); } - function addNodeWithRecursiveChild(node: Node, child: Node): void { + function addNodeWithRecursiveChild(node: Node, child: Node, addChildrenRecursively: (node: Node) => void): void { startNode(node); addChildrenRecursively(child); endNode(); } /** Look for navigation bar items in node's subtree, adding them to the current `parent`. */ - function addChildrenRecursively(node: Node): void { - if (!node || isToken(node)) { - return; - } + function addChildrenRecursively(node: Node, cancellationToken: CancellationToken): void { + function addChildrenRecursively(node: Node): void { + cancellationToken.throwIfCancellationRequested(); - switch (node.kind) { - case SyntaxKind.Constructor: - // Get parameter properties, and treat them as being on the *same* level as the constructor, not under it. - const ctr = node; - addNodeWithRecursiveChild(ctr, ctr.body); + if (!node || isToken(node)) { + return; + } - // Parameter properties are children of the class, not the constructor. - for (const param of ctr.parameters) { - if (isParameterPropertyDeclaration(param)) { - addLeafNode(param); + switch (node.kind) { + case SyntaxKind.Constructor: + // Get parameter properties, and treat them as being on the *same* level as the constructor, not under it. + const ctr = node; + addNodeWithRecursiveChild(ctr, ctr.body, addChildrenRecursively); + + // Parameter properties are children of the class, not the constructor. + for (const param of ctr.parameters) { + if (isParameterPropertyDeclaration(param)) { + addLeafNode(param); + } } - } - break; + break; - case SyntaxKind.MethodDeclaration: - case SyntaxKind.GetAccessor: - case SyntaxKind.SetAccessor: - case SyntaxKind.MethodSignature: - if (!hasDynamicName((node))) { - addNodeWithRecursiveChild(node, (node).body); - } - break; + case SyntaxKind.MethodDeclaration: + case SyntaxKind.GetAccessor: + case SyntaxKind.SetAccessor: + case SyntaxKind.MethodSignature: + if (!hasDynamicName((node))) { + addNodeWithRecursiveChild(node, (node).body, addChildrenRecursively); + } + break; - case SyntaxKind.PropertyDeclaration: - case SyntaxKind.PropertySignature: - if (!hasDynamicName((node))) { - addLeafNode(node); - } - break; + case SyntaxKind.PropertyDeclaration: + case SyntaxKind.PropertySignature: + if (!hasDynamicName((node))) { + addLeafNode(node); + } + break; - case SyntaxKind.ImportClause: - const importClause = node; - // Handle default import case e.g.: - // import d from "mod"; - if (importClause.name) { - addLeafNode(importClause); - } + case SyntaxKind.ImportClause: + const importClause = node; + // Handle default import case e.g.: + // import d from "mod"; + if (importClause.name) { + addLeafNode(importClause); + } - // Handle named bindings in imports e.g.: - // import * as NS from "mod"; - // import {a, b as B} from "mod"; - const {namedBindings} = importClause; - if (namedBindings) { - if (namedBindings.kind === SyntaxKind.NamespaceImport) { - addLeafNode(namedBindings); + // Handle named bindings in imports e.g.: + // import * as NS from "mod"; + // import {a, b as B} from "mod"; + const {namedBindings} = importClause; + if (namedBindings) { + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + addLeafNode(namedBindings); + } + else { + for (const element of (namedBindings).elements) { + addLeafNode(element); + } + } + } + break; + + case SyntaxKind.BindingElement: + case SyntaxKind.VariableDeclaration: + const decl = node; + const name = decl.name; + if (isBindingPattern(name)) { + addChildrenRecursively(name); + } + else if (decl.initializer && isFunctionOrClassExpression(decl.initializer)) { + // For `const x = function() {}`, just use the function node, not the const. + addChildrenRecursively(decl.initializer); } else { - for (const element of (namedBindings).elements) { - addLeafNode(element); + addNodeWithRecursiveChild(decl, decl.initializer, addChildrenRecursively); + } + break; + + case SyntaxKind.ArrowFunction: + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.FunctionExpression: + addNodeWithRecursiveChild(node, (node).body, addChildrenRecursively); + break; + + case SyntaxKind.EnumDeclaration: + startNode(node); + for (const member of (node).members) { + if (!isComputedProperty(member)) { + addLeafNode(member); } } - } - break; + endNode(); + break; - case SyntaxKind.BindingElement: - case SyntaxKind.VariableDeclaration: - const decl = node; - const name = decl.name; - if (isBindingPattern(name)) { - addChildrenRecursively(name); - } - else if (decl.initializer && isFunctionOrClassExpression(decl.initializer)) { - // For `const x = function() {}`, just use the function node, not the const. - addChildrenRecursively(decl.initializer); - } - else { - addNodeWithRecursiveChild(decl, decl.initializer); - } - break; - - case SyntaxKind.ArrowFunction: - case SyntaxKind.FunctionDeclaration: - case SyntaxKind.FunctionExpression: - addNodeWithRecursiveChild(node, (node).body); - break; - - case SyntaxKind.EnumDeclaration: - startNode(node); - for (const member of (node).members) { - if (!isComputedProperty(member)) { - addLeafNode(member); + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ClassExpression: + case SyntaxKind.InterfaceDeclaration: + startNode(node); + for (const member of (node).members) { + addChildrenRecursively(member); } - } - endNode(); - break; + endNode(); + break; - case SyntaxKind.ClassDeclaration: - case SyntaxKind.ClassExpression: - case SyntaxKind.InterfaceDeclaration: - startNode(node); - for (const member of (node).members) { - addChildrenRecursively(member); - } - endNode(); - break; + case SyntaxKind.ModuleDeclaration: + addNodeWithRecursiveChild(node, getInteriorModule(node).body, addChildrenRecursively); + break; - case SyntaxKind.ModuleDeclaration: - addNodeWithRecursiveChild(node, getInteriorModule(node).body); - break; + case SyntaxKind.ExportSpecifier: + case SyntaxKind.ImportEqualsDeclaration: + case SyntaxKind.IndexSignature: + case SyntaxKind.CallSignature: + case SyntaxKind.ConstructSignature: + case SyntaxKind.TypeAliasDeclaration: + addLeafNode(node); + break; - case SyntaxKind.ExportSpecifier: - case SyntaxKind.ImportEqualsDeclaration: - case SyntaxKind.IndexSignature: - case SyntaxKind.CallSignature: - case SyntaxKind.ConstructSignature: - case SyntaxKind.TypeAliasDeclaration: - addLeafNode(node); - break; - - default: - forEach(node.jsDoc, jsDoc => { - forEach(jsDoc.tags, tag => { - if (tag.kind === SyntaxKind.JSDocTypedefTag) { - addLeafNode(tag); - } + default: + forEach(node.jsDoc, jsDoc => { + forEach(jsDoc.tags, tag => { + if (tag.kind === SyntaxKind.JSDocTypedefTag) { + addLeafNode(tag); + } + }); }); - }); - forEachChild(node, addChildrenRecursively); + forEachChild(node, addChildrenRecursively); + } } + + addChildrenRecursively(node); } /** Merge declarations of the same kind. */ diff --git a/src/services/outliningElementsCollector.ts b/src/services/outliningElementsCollector.ts index 2ad20a7ed0..96a7c9a3c4 100644 --- a/src/services/outliningElementsCollector.ts +++ b/src/services/outliningElementsCollector.ts @@ -1,6 +1,6 @@ /* @internal */ namespace ts.OutliningElementsCollector { - export function collectElements(sourceFile: SourceFile): OutliningSpan[] { + export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] { const elements: OutliningSpan[] = []; const collapseText = "..."; @@ -38,6 +38,7 @@ namespace ts.OutliningElementsCollector { let singleLineCommentCount = 0; for (const currentComment of comments) { + cancellationToken.throwIfCancellationRequested(); // For single line comments, combine consecutive ones (2 or more) into // a single span from the start of the first till the end of the last @@ -84,6 +85,7 @@ namespace ts.OutliningElementsCollector { let depth = 0; const maxDepth = 20; function walk(n: Node): void { + cancellationToken.throwIfCancellationRequested(); if (depth > maxDepth) { return; } diff --git a/src/services/services.ts b/src/services/services.ts index c1b719d347..cef307eb8f 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1541,11 +1541,11 @@ namespace ts { } function getNavigationBarItems(fileName: string): NavigationBarItem[] { - return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName)); + return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); } function getNavigationTree(fileName: string): NavigationTree { - return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName)); + return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); } function isTsOrTsxFile(fileName: string): boolean { @@ -1584,7 +1584,7 @@ namespace ts { function getOutliningSpans(fileName: string): OutliningSpan[] { // doesn't use compiler - no need to synchronize with host const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName); - return OutliningElementsCollector.collectElements(sourceFile); + return OutliningElementsCollector.collectElements(sourceFile, cancellationToken); } function getBraceMatchingAtPosition(fileName: string, position: number) { From a37053f780583e979386a8ff7802e51e64e5bf67 Mon Sep 17 00:00:00 2001 From: Jason Ramsay Date: Wed, 22 Feb 2017 09:54:45 -0800 Subject: [PATCH 2/5] Addressing CR comments - Adding a throttle - Refactor - Navbar reset onCancel --- src/compiler/program.ts | 52 +++++----- src/compiler/types.ts | 2 + .../unittests/tsserverProjectSystem.ts | 94 ++++++++++--------- src/services/navigationBar.ts | 17 ++-- src/services/outliningElementsCollector.ts | 2 +- src/services/services.ts | 48 +++++++++- src/services/shims.ts | 23 ----- src/services/types.ts | 1 + 8 files changed, 137 insertions(+), 102 deletions(-) diff --git a/src/compiler/program.ts b/src/compiler/program.ts index 62f3c06644..083c812464 100644 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -72,6 +72,20 @@ namespace ts { return getNormalizedPathFromPathComponents(commonPathComponents); } + /* @internal */ + export function runWithCancellationToken(func: () => T, onCancel?: () => void): T { + try { + return func(); + } + catch (e) { + if (e instanceof OperationCanceledException && onCancel) { + // We were canceled while performing the operation. + onCancel(); + } + throw e; + } + } + interface OutputFingerprint { hash: string; byteOrderMark: boolean; @@ -873,29 +887,19 @@ namespace ts { return sourceFile.parseDiagnostics; } - function runWithCancellationToken(func: () => T): T { - try { - return func(); - } - catch (e) { - if (e instanceof OperationCanceledException) { - // We were canceled while performing the operation. Because our type checker - // might be a bad state, we need to throw it away. - // - // Note: we are overly aggressive here. We do not actually *have* to throw away - // the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep - // the lifetimes of these two TypeCheckers the same. Also, we generally only - // cancel when the user has made a change anyways. And, in that case, we (the - // program instance) will get thrown away anyways. So trying to keep one of - // these type checkers alive doesn't serve much purpose. - noDiagnosticsTypeChecker = undefined; - diagnosticsProducingTypeChecker = undefined; - } - - throw e; - } + function onCancel() { + // Because our type checker might be a bad state, we need to throw it away. + // Note: we are overly aggressive here. We do not actually *have* to throw away + // the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep + // the lifetimes of these two TypeCheckers the same. Also, we generally only + // cancel when the user has made a change anyways. And, in that case, we (the + // program instance) will get thrown away anyways. So trying to keep one of + // these type checkers alive doesn't serve much purpose. + noDiagnosticsTypeChecker = undefined; + diagnosticsProducingTypeChecker = undefined; } + function getSemanticDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { return runWithCancellationToken(() => { const typeChecker = getDiagnosticsProducingTypeChecker(); @@ -910,7 +914,7 @@ namespace ts { const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName); return bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile); - }); + }, onCancel); } function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): Diagnostic[] { @@ -1089,7 +1093,7 @@ namespace ts { function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic { return createDiagnosticForNodeInSourceFile(sourceFile, node, message, arg0, arg1, arg2); } - }); + }, onCancel); } function getDeclarationDiagnosticsWorker(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { @@ -1097,7 +1101,7 @@ namespace ts { const resolver = getDiagnosticsProducingTypeChecker().getEmitResolver(sourceFile, cancellationToken); // Don't actually write any files since we're just getting diagnostics. return ts.getDeclarationDiagnostics(getEmitHost(noop), resolver, sourceFile); - }); + }, onCancel); } function getDeclarationDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 7fc77b4870..17adbd2ea3 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2262,6 +2262,8 @@ /** @throws OperationCanceledException if isCancellationRequested is true */ throwIfCancellationRequested(): void; + + throttleWaitMilliseconds?: number; } export interface Program extends ScriptReferenceHost { diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index 01e484fd4b..ee4273c3d2 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -547,6 +547,45 @@ namespace ts.projectSystem { readonly getEnvironmentVariable = notImplemented; } + export class TestServerCancellationToken implements server.ServerCancellationToken { + private currentId = -1; + private requestToCancel = -1; + private isCancellationRequestedCount = 0; + + constructor(private cancelAfterRequest = 0) { + } + + get throttleWaitMilliseconds() { + // For testing purposes disable the throttle + return 0; + } + + setRequest(requestId: number) { + this.currentId = requestId; + } + + setRequestToCancel(requestId: number) { + this.resetToken(); + this.requestToCancel = requestId; + } + + resetRequest(requestId: number) { + assert.equal(requestId, this.currentId, "unexpected request id in cancellation") + this.currentId = undefined; + } + + isCancellationRequested() { + this.isCancellationRequestedCount++; + return this.requestToCancel === this.currentId && this.isCancellationRequestedCount >= this.cancelAfterRequest; + } + + resetToken() { + this.currentId = -1; + this.isCancellationRequestedCount = 0; + this.requestToCancel = -1; + } + } + export function makeSessionRequest(command: string, args: T) { const newRequest: protocol.Request = { seq: 0, @@ -3324,22 +3363,7 @@ namespace ts.projectSystem { }) }; - let requestToCancel = -1; - const cancellationToken: server.ServerCancellationToken = (function(){ - let currentId: number; - return { - setRequest(requestId) { - currentId = requestId; - }, - resetRequest(requestId) { - assert.equal(requestId, currentId, "unexpected request id in cancellation") - currentId = undefined; - }, - isCancellationRequested() { - return requestToCancel === currentId; - } - } - })(); + const cancellationToken = new TestServerCancellationToken(); const host = createServerHost([f1, config]); const session = createSession(host, /*typingsInstaller*/ undefined, () => {}, cancellationToken); { @@ -3374,13 +3398,13 @@ namespace ts.projectSystem { host.clearOutput(); // cancel previously issued Geterr - requestToCancel = getErrId; + cancellationToken.setRequestToCancel(getErrId); host.runQueuedTimeoutCallbacks(); assert.equal(host.getOutput().length, 1, "expect 1 message"); verifyRequestCompleted(getErrId, 0); - requestToCancel = -1; + cancellationToken.resetToken(); } { const getErrId = session.getNextSeq(); @@ -3397,12 +3421,12 @@ namespace ts.projectSystem { assert.equal(e1.event, "syntaxDiag"); host.clearOutput(); - requestToCancel = getErrId; + cancellationToken.setRequestToCancel(getErrId); host.runQueuedImmediateCallbacks(); assert.equal(host.getOutput().length, 1, "expect 1 message"); verifyRequestCompleted(getErrId, 0); - requestToCancel = -1; + cancellationToken.resetToken(); } { const getErrId = session.getNextSeq(); @@ -3425,7 +3449,7 @@ namespace ts.projectSystem { assert.equal(e2.event, "semanticDiag"); verifyRequestCompleted(getErrId, 1); - requestToCancel = -1; + cancellationToken.resetToken(); } { const getErr1 = session.getNextSeq(); @@ -3472,26 +3496,8 @@ namespace ts.projectSystem { }) }; - let requestToCancel = -1; - let isCancellationRequestedCount = 0; - let cancelAfterRequest = 3; let operationCanceledExceptionThrown = false; - const cancellationToken: server.ServerCancellationToken = (function () { - let currentId: number; - return { - setRequest(requestId) { - currentId = requestId; - }, - resetRequest(requestId) { - assert.equal(requestId, currentId, "unexpected request id in cancellation") - currentId = undefined; - }, - isCancellationRequested() { - isCancellationRequestedCount++; - return requestToCancel === currentId && isCancellationRequestedCount >= cancelAfterRequest; - } - } - })(); + const cancellationToken = new TestServerCancellationToken(/*cancelAfterRequest*/ 3); const host = createServerHost([f1, config]); const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken); { @@ -3529,17 +3535,17 @@ namespace ts.projectSystem { // Set the next request to be cancellable // The cancellation token will cancel the request the third time // isCancellationRequested() is called. - requestToCancel = session.getNextSeq(); - isCancellationRequestedCount = 0; + cancellationToken.setRequestToCancel(session.getNextSeq()); operationCanceledExceptionThrown = false; try { session.executeCommandSeq(request); - } catch (e) { + } + catch (e) { assert(e instanceof OperationCanceledException); operationCanceledExceptionThrown = true; } - assert(operationCanceledExceptionThrown); + assert(operationCanceledExceptionThrown, "Operation Canceled Exception not thrown for request: " + JSON.stringify(request)); } }); }); diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 37b6bedbd5..2910a3528d 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -14,21 +14,24 @@ namespace ts.NavigationBar { indent: number; // # of parents } - export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarItem[] { + export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationBarItem[] { + curCancellationToken = cancellationToken; curSourceFile = sourceFile; - const result = map(topLevelItems(rootNavigationBarNode(sourceFile, cancellationToken)), convertToTopLevelItem); + const result = runWithCancellationToken(() => map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem), () => curSourceFile = undefined); curSourceFile = undefined; return result; } - export function getNavigationTree(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationTree { + export function getNavigationTree(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationTree { + curCancellationToken = cancellationToken; curSourceFile = sourceFile; - const result = convertToTree(rootNavigationBarNode(sourceFile, cancellationToken)); + const result = runWithCancellationToken(() => convertToTree(rootNavigationBarNode(sourceFile)), () => curSourceFile = undefined); curSourceFile = undefined; return result; } // Keep sourceFile handy so we don't have to search for it every time we need to call `getText`. + let curCancellationToken: ThrottledCancellationToken; let curSourceFile: SourceFile; function nodeText(node: Node): string { return node.getText(curSourceFile); @@ -55,12 +58,12 @@ namespace ts.NavigationBar { const parentsStack: NavigationBarNode[] = []; let parent: NavigationBarNode; - function rootNavigationBarNode(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarNode { + function rootNavigationBarNode(sourceFile: SourceFile): NavigationBarNode { Debug.assert(!parentsStack.length); const root: NavigationBarNode = { node: sourceFile, additionalNodes: undefined, parent: undefined, children: undefined, indent: 0 }; parent = root; for (const statement of sourceFile.statements) { - addChildrenRecursively(statement, cancellationToken); + addChildrenRecursively(statement, curCancellationToken); } endNode(); Debug.assert(!parent && !parentsStack.length); @@ -110,7 +113,7 @@ namespace ts.NavigationBar { } /** Look for navigation bar items in node's subtree, adding them to the current `parent`. */ - function addChildrenRecursively(node: Node, cancellationToken: CancellationToken): void { + function addChildrenRecursively(node: Node, cancellationToken: ThrottledCancellationToken): void { function addChildrenRecursively(node: Node): void { cancellationToken.throwIfCancellationRequested(); diff --git a/src/services/outliningElementsCollector.ts b/src/services/outliningElementsCollector.ts index 96a7c9a3c4..1b7618c032 100644 --- a/src/services/outliningElementsCollector.ts +++ b/src/services/outliningElementsCollector.ts @@ -1,6 +1,6 @@ /* @internal */ namespace ts.OutliningElementsCollector { - export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] { + export function collectElements(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): OutliningSpan[] { const elements: OutliningSpan[] = []; const collapseText = "..."; diff --git a/src/services/services.ts b/src/services/services.ts index cef307eb8f..6cf50940fb 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -958,7 +958,12 @@ namespace ts { } class CancellationTokenObject implements CancellationToken { + throttleWaitMilliseconds?: number; + constructor(private cancellationToken: HostCancellationToken) { + if (cancellationToken.throttleWaitMilliseconds !== undefined) { + this.throttleWaitMilliseconds = cancellationToken.throttleWaitMilliseconds; + } } public isCancellationRequested() { @@ -972,6 +977,42 @@ namespace ts { } } + /** A cancellation that throttles calls to the host */ + export class ThrottledCancellationToken implements CancellationToken { + // Store when we last tried to cancel. Checking cancellation can be expensive (as we have + // to marshall over to the host layer). So we only bother actually checking once enough + // time has passed. + private lastCancellationCheckTime = 0; + // The minuimum duration to wait in milliseconds before querying host. + // The default of 10 milliseconds will be used unless throttleWaitMilliseconds + // is specified in the host cancellation token. + throttleWaitMilliseconds: number = 10; + + constructor(private hostCancellationToken: HostCancellationToken) { + if (hostCancellationToken.throttleWaitMilliseconds !== undefined) { + this.throttleWaitMilliseconds = hostCancellationToken.throttleWaitMilliseconds; + } + } + + public isCancellationRequested(): boolean { + const time = timestamp(); + const duration = Math.abs(time - this.lastCancellationCheckTime); + if (duration >= this.throttleWaitMilliseconds) { + // Check no more than the min wait in milliseconds + this.lastCancellationCheckTime = time; + return this.hostCancellationToken.isCancellationRequested(); + } + + return false; + } + + public throwIfCancellationRequested(): void { + if (this.isCancellationRequested()) { + throw new OperationCanceledException(); + } + } + } + export function createLanguageService(host: LanguageServiceHost, documentRegistry: DocumentRegistry = createDocumentRegistry(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(), host.getCurrentDirectory())): LanguageService { @@ -984,6 +1025,7 @@ namespace ts { const useCaseSensitivefileNames = host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(); const cancellationToken = new CancellationTokenObject(host.getCancellationToken && host.getCancellationToken()); + const throttledCancellationToken = new ThrottledCancellationToken(cancellationToken); const currentDirectory = host.getCurrentDirectory(); // Check if the localized messages json is set, otherwise query the host for it @@ -1541,11 +1583,11 @@ namespace ts { } function getNavigationBarItems(fileName: string): NavigationBarItem[] { - return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); + return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName), throttledCancellationToken); } function getNavigationTree(fileName: string): NavigationTree { - return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); + return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName), throttledCancellationToken); } function isTsOrTsxFile(fileName: string): boolean { @@ -1584,7 +1626,7 @@ namespace ts { function getOutliningSpans(fileName: string): OutliningSpan[] { // doesn't use compiler - no need to synchronize with host const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName); - return OutliningElementsCollector.collectElements(sourceFile, cancellationToken); + return OutliningElementsCollector.collectElements(sourceFile, throttledCancellationToken); } function getBraceMatchingAtPosition(fileName: string, position: number) { diff --git a/src/services/shims.ts b/src/services/shims.ts index 6fe9aed144..de2fd39b8a 100644 --- a/src/services/shims.ts +++ b/src/services/shims.ts @@ -469,29 +469,6 @@ namespace ts { } } - /** A cancellation that throttles calls to the host */ - class ThrottledCancellationToken implements HostCancellationToken { - // Store when we last tried to cancel. Checking cancellation can be expensive (as we have - // to marshall over to the host layer). So we only bother actually checking once enough - // time has passed. - private lastCancellationCheckTime = 0; - - constructor(private hostCancellationToken: HostCancellationToken) { - } - - public isCancellationRequested(): boolean { - const time = timestamp(); - const duration = Math.abs(time - this.lastCancellationCheckTime); - if (duration > 10) { - // Check no more than once every 10 ms. - this.lastCancellationCheckTime = time; - return this.hostCancellationToken.isCancellationRequested(); - } - - return false; - } - } - export class CoreServicesShimHostAdapter implements ParseConfigHost, ModuleResolutionHost { public directoryExists: (directoryName: string) => boolean; diff --git a/src/services/types.ts b/src/services/types.ts index a6ca385259..f9c33e15df 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -121,6 +121,7 @@ namespace ts { export interface HostCancellationToken { isCancellationRequested(): boolean; + throttleWaitMilliseconds?: number; } // From 497d8d3a58f40729941d08de64ee082e9b0d8154 Mon Sep 17 00:00:00 2001 From: Jason Ramsay Date: Wed, 22 Feb 2017 15:33:57 -0800 Subject: [PATCH 3/5] Updates from CR comments --- src/compiler/program.ts | 38 ++++++++++++++++++++++------------- src/services/navigationBar.ts | 26 +++++++++++++++--------- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/compiler/program.ts b/src/compiler/program.ts index 083c812464..b94efe1822 100644 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -887,18 +887,28 @@ namespace ts { return sourceFile.parseDiagnostics; } - function onCancel() { - // Because our type checker might be a bad state, we need to throw it away. - // Note: we are overly aggressive here. We do not actually *have* to throw away - // the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep - // the lifetimes of these two TypeCheckers the same. Also, we generally only - // cancel when the user has made a change anyways. And, in that case, we (the - // program instance) will get thrown away anyways. So trying to keep one of - // these type checkers alive doesn't serve much purpose. - noDiagnosticsTypeChecker = undefined; - diagnosticsProducingTypeChecker = undefined; - } + function runWithCancellationToken(func: () => T): T { + try { + return func(); + } + catch (e) { + if (e instanceof OperationCanceledException) { + // We were canceled while performing the operation. Because our type checker + // might be a bad state, we need to throw it away. + // + // Note: we are overly aggressive here. We do not actually *have* to throw away + // the "noDiagnosticsTypeChecker". However, for simplicity, i'd like to keep + // the lifetimes of these two TypeCheckers the same. Also, we generally only + // cancel when the user has made a change anyways. And, in that case, we (the + // program instance) will get thrown away anyways. So trying to keep one of + // these type checkers alive doesn't serve much purpose. + noDiagnosticsTypeChecker = undefined; + diagnosticsProducingTypeChecker = undefined; + } + throw e; + } + } function getSemanticDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { return runWithCancellationToken(() => { @@ -914,7 +924,7 @@ namespace ts { const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName); return bindDiagnostics.concat(checkDiagnostics, fileProcessingDiagnosticsInFile, programDiagnosticsInFile); - }, onCancel); + }); } function getJavaScriptSyntacticDiagnosticsForFile(sourceFile: SourceFile): Diagnostic[] { @@ -1093,7 +1103,7 @@ namespace ts { function createDiagnosticForNode(node: Node, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number): Diagnostic { return createDiagnosticForNodeInSourceFile(sourceFile, node, message, arg0, arg1, arg2); } - }, onCancel); + }); } function getDeclarationDiagnosticsWorker(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { @@ -1101,7 +1111,7 @@ namespace ts { const resolver = getDiagnosticsProducingTypeChecker().getEmitResolver(sourceFile, cancellationToken); // Don't actually write any files since we're just getting diagnostics. return ts.getDeclarationDiagnostics(getEmitHost(noop), resolver, sourceFile); - }, onCancel); + }); } function getDeclarationDiagnosticsForFile(sourceFile: SourceFile, cancellationToken: CancellationToken): Diagnostic[] { diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 2910a3528d..62ead677d8 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -17,21 +17,27 @@ namespace ts.NavigationBar { export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationBarItem[] { curCancellationToken = cancellationToken; curSourceFile = sourceFile; - const result = runWithCancellationToken(() => map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem), () => curSourceFile = undefined); - curSourceFile = undefined; - return result; + try { + return map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem); + } + finally { + curSourceFile = undefined; + } } export function getNavigationTree(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationTree { curCancellationToken = cancellationToken; curSourceFile = sourceFile; - const result = runWithCancellationToken(() => convertToTree(rootNavigationBarNode(sourceFile)), () => curSourceFile = undefined); - curSourceFile = undefined; - return result; + try { + return convertToTree(rootNavigationBarNode(sourceFile)); + } + finally { + curSourceFile = undefined; + } } // Keep sourceFile handy so we don't have to search for it every time we need to call `getText`. - let curCancellationToken: ThrottledCancellationToken; + let curCancellationToken: CancellationToken; let curSourceFile: SourceFile; function nodeText(node: Node): string { return node.getText(curSourceFile); @@ -63,7 +69,7 @@ namespace ts.NavigationBar { const root: NavigationBarNode = { node: sourceFile, additionalNodes: undefined, parent: undefined, children: undefined, indent: 0 }; parent = root; for (const statement of sourceFile.statements) { - addChildrenRecursively(statement, curCancellationToken); + addChildrenRecursively(statement); } endNode(); Debug.assert(!parent && !parentsStack.length); @@ -113,9 +119,9 @@ namespace ts.NavigationBar { } /** Look for navigation bar items in node's subtree, adding them to the current `parent`. */ - function addChildrenRecursively(node: Node, cancellationToken: ThrottledCancellationToken): void { + function addChildrenRecursively(node: Node): void { function addChildrenRecursively(node: Node): void { - cancellationToken.throwIfCancellationRequested(); + curCancellationToken.throwIfCancellationRequested(); if (!node || isToken(node)) { return; From e62108cf9b5dbf678bf596def315ab766b82fa25 Mon Sep 17 00:00:00 2001 From: Jason Ramsay Date: Wed, 22 Feb 2017 17:33:11 -0800 Subject: [PATCH 4/5] Removing throttling until tests prove it is required --- src/compiler/program.ts | 14 - src/compiler/types.ts | 2 - .../unittests/tsserverProjectSystem.ts | 9 +- src/services/navigationBar.ts | 258 +++++++++--------- src/services/outliningElementsCollector.ts | 2 +- src/services/services.ts | 48 +--- src/services/shims.ts | 23 ++ src/services/types.ts | 1 - 8 files changed, 156 insertions(+), 201 deletions(-) diff --git a/src/compiler/program.ts b/src/compiler/program.ts index b94efe1822..62f3c06644 100644 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -72,20 +72,6 @@ namespace ts { return getNormalizedPathFromPathComponents(commonPathComponents); } - /* @internal */ - export function runWithCancellationToken(func: () => T, onCancel?: () => void): T { - try { - return func(); - } - catch (e) { - if (e instanceof OperationCanceledException && onCancel) { - // We were canceled while performing the operation. - onCancel(); - } - throw e; - } - } - interface OutputFingerprint { hash: string; byteOrderMark: boolean; diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 17adbd2ea3..7fc77b4870 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2262,8 +2262,6 @@ /** @throws OperationCanceledException if isCancellationRequested is true */ throwIfCancellationRequested(): void; - - throttleWaitMilliseconds?: number; } export interface Program extends ScriptReferenceHost { diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index ee4273c3d2..d9abb0a907 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -555,11 +555,6 @@ namespace ts.projectSystem { constructor(private cancelAfterRequest = 0) { } - get throttleWaitMilliseconds() { - // For testing purposes disable the throttle - return 0; - } - setRequest(requestId: number) { this.currentId = requestId; } @@ -3495,8 +3490,6 @@ namespace ts.projectSystem { compilerOptions: {} }) }; - - let operationCanceledExceptionThrown = false; const cancellationToken = new TestServerCancellationToken(/*cancelAfterRequest*/ 3); const host = createServerHost([f1, config]); const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken); @@ -3536,7 +3529,7 @@ namespace ts.projectSystem { // The cancellation token will cancel the request the third time // isCancellationRequested() is called. cancellationToken.setRequestToCancel(session.getNextSeq()); - operationCanceledExceptionThrown = false; + let operationCanceledExceptionThrown = false; try { session.executeCommandSeq(request); diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 62ead677d8..811762fb73 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -14,7 +14,7 @@ namespace ts.NavigationBar { indent: number; // # of parents } - export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationBarItem[] { + export function getNavigationBarItems(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationBarItem[] { curCancellationToken = cancellationToken; curSourceFile = sourceFile; try { @@ -22,10 +22,11 @@ namespace ts.NavigationBar { } finally { curSourceFile = undefined; + curCancellationToken = undefined; } } - export function getNavigationTree(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): NavigationTree { + export function getNavigationTree(sourceFile: SourceFile, cancellationToken: CancellationToken): NavigationTree { curCancellationToken = cancellationToken; curSourceFile = sourceFile; try { @@ -33,6 +34,7 @@ namespace ts.NavigationBar { } finally { curSourceFile = undefined; + curCancellationToken = undefined; } } @@ -112,7 +114,7 @@ namespace ts.NavigationBar { parent = parentsStack.pop(); } - function addNodeWithRecursiveChild(node: Node, child: Node, addChildrenRecursively: (node: Node) => void): void { + function addNodeWithRecursiveChild(node: Node, child: Node): void { startNode(node); addChildrenRecursively(child); endNode(); @@ -120,136 +122,132 @@ namespace ts.NavigationBar { /** Look for navigation bar items in node's subtree, adding them to the current `parent`. */ function addChildrenRecursively(node: Node): void { - function addChildrenRecursively(node: Node): void { - curCancellationToken.throwIfCancellationRequested(); + curCancellationToken.throwIfCancellationRequested(); - if (!node || isToken(node)) { - return; - } - - switch (node.kind) { - case SyntaxKind.Constructor: - // Get parameter properties, and treat them as being on the *same* level as the constructor, not under it. - const ctr = node; - addNodeWithRecursiveChild(ctr, ctr.body, addChildrenRecursively); - - // Parameter properties are children of the class, not the constructor. - for (const param of ctr.parameters) { - if (isParameterPropertyDeclaration(param)) { - addLeafNode(param); - } - } - break; - - case SyntaxKind.MethodDeclaration: - case SyntaxKind.GetAccessor: - case SyntaxKind.SetAccessor: - case SyntaxKind.MethodSignature: - if (!hasDynamicName((node))) { - addNodeWithRecursiveChild(node, (node).body, addChildrenRecursively); - } - break; - - case SyntaxKind.PropertyDeclaration: - case SyntaxKind.PropertySignature: - if (!hasDynamicName((node))) { - addLeafNode(node); - } - break; - - case SyntaxKind.ImportClause: - const importClause = node; - // Handle default import case e.g.: - // import d from "mod"; - if (importClause.name) { - addLeafNode(importClause); - } - - // Handle named bindings in imports e.g.: - // import * as NS from "mod"; - // import {a, b as B} from "mod"; - const {namedBindings} = importClause; - if (namedBindings) { - if (namedBindings.kind === SyntaxKind.NamespaceImport) { - addLeafNode(namedBindings); - } - else { - for (const element of (namedBindings).elements) { - addLeafNode(element); - } - } - } - break; - - case SyntaxKind.BindingElement: - case SyntaxKind.VariableDeclaration: - const decl = node; - const name = decl.name; - if (isBindingPattern(name)) { - addChildrenRecursively(name); - } - else if (decl.initializer && isFunctionOrClassExpression(decl.initializer)) { - // For `const x = function() {}`, just use the function node, not the const. - addChildrenRecursively(decl.initializer); - } - else { - addNodeWithRecursiveChild(decl, decl.initializer, addChildrenRecursively); - } - break; - - case SyntaxKind.ArrowFunction: - case SyntaxKind.FunctionDeclaration: - case SyntaxKind.FunctionExpression: - addNodeWithRecursiveChild(node, (node).body, addChildrenRecursively); - break; - - case SyntaxKind.EnumDeclaration: - startNode(node); - for (const member of (node).members) { - if (!isComputedProperty(member)) { - addLeafNode(member); - } - } - endNode(); - break; - - case SyntaxKind.ClassDeclaration: - case SyntaxKind.ClassExpression: - case SyntaxKind.InterfaceDeclaration: - startNode(node); - for (const member of (node).members) { - addChildrenRecursively(member); - } - endNode(); - break; - - case SyntaxKind.ModuleDeclaration: - addNodeWithRecursiveChild(node, getInteriorModule(node).body, addChildrenRecursively); - break; - - case SyntaxKind.ExportSpecifier: - case SyntaxKind.ImportEqualsDeclaration: - case SyntaxKind.IndexSignature: - case SyntaxKind.CallSignature: - case SyntaxKind.ConstructSignature: - case SyntaxKind.TypeAliasDeclaration: - addLeafNode(node); - break; - - default: - forEach(node.jsDoc, jsDoc => { - forEach(jsDoc.tags, tag => { - if (tag.kind === SyntaxKind.JSDocTypedefTag) { - addLeafNode(tag); - } - }); - }); - - forEachChild(node, addChildrenRecursively); - } + if (!node || isToken(node)) { + return; } - addChildrenRecursively(node); + switch (node.kind) { + case SyntaxKind.Constructor: + // Get parameter properties, and treat them as being on the *same* level as the constructor, not under it. + const ctr = node; + addNodeWithRecursiveChild(ctr, ctr.body); + + // Parameter properties are children of the class, not the constructor. + for (const param of ctr.parameters) { + if (isParameterPropertyDeclaration(param)) { + addLeafNode(param); + } + } + break; + + case SyntaxKind.MethodDeclaration: + case SyntaxKind.GetAccessor: + case SyntaxKind.SetAccessor: + case SyntaxKind.MethodSignature: + if (!hasDynamicName((node))) { + addNodeWithRecursiveChild(node, (node).body); + } + break; + + case SyntaxKind.PropertyDeclaration: + case SyntaxKind.PropertySignature: + if (!hasDynamicName((node))) { + addLeafNode(node); + } + break; + + case SyntaxKind.ImportClause: + const importClause = node; + // Handle default import case e.g.: + // import d from "mod"; + if (importClause.name) { + addLeafNode(importClause); + } + + // Handle named bindings in imports e.g.: + // import * as NS from "mod"; + // import {a, b as B} from "mod"; + const {namedBindings} = importClause; + if (namedBindings) { + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + addLeafNode(namedBindings); + } + else { + for (const element of (namedBindings).elements) { + addLeafNode(element); + } + } + } + break; + + case SyntaxKind.BindingElement: + case SyntaxKind.VariableDeclaration: + const decl = node; + const name = decl.name; + if (isBindingPattern(name)) { + addChildrenRecursively(name); + } + else if (decl.initializer && isFunctionOrClassExpression(decl.initializer)) { + // For `const x = function() {}`, just use the function node, not the const. + addChildrenRecursively(decl.initializer); + } + else { + addNodeWithRecursiveChild(decl, decl.initializer); + } + break; + + case SyntaxKind.ArrowFunction: + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.FunctionExpression: + addNodeWithRecursiveChild(node, (node).body); + break; + + case SyntaxKind.EnumDeclaration: + startNode(node); + for (const member of (node).members) { + if (!isComputedProperty(member)) { + addLeafNode(member); + } + } + endNode(); + break; + + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ClassExpression: + case SyntaxKind.InterfaceDeclaration: + startNode(node); + for (const member of (node).members) { + addChildrenRecursively(member); + } + endNode(); + break; + + case SyntaxKind.ModuleDeclaration: + addNodeWithRecursiveChild(node, getInteriorModule(node).body); + break; + + case SyntaxKind.ExportSpecifier: + case SyntaxKind.ImportEqualsDeclaration: + case SyntaxKind.IndexSignature: + case SyntaxKind.CallSignature: + case SyntaxKind.ConstructSignature: + case SyntaxKind.TypeAliasDeclaration: + addLeafNode(node); + break; + + default: + forEach(node.jsDoc, jsDoc => { + forEach(jsDoc.tags, tag => { + if (tag.kind === SyntaxKind.JSDocTypedefTag) { + addLeafNode(tag); + } + }); + }); + + forEachChild(node, addChildrenRecursively); + } } /** Merge declarations of the same kind. */ diff --git a/src/services/outliningElementsCollector.ts b/src/services/outliningElementsCollector.ts index 1b7618c032..96a7c9a3c4 100644 --- a/src/services/outliningElementsCollector.ts +++ b/src/services/outliningElementsCollector.ts @@ -1,6 +1,6 @@ /* @internal */ namespace ts.OutliningElementsCollector { - export function collectElements(sourceFile: SourceFile, cancellationToken: ThrottledCancellationToken): OutliningSpan[] { + export function collectElements(sourceFile: SourceFile, cancellationToken: CancellationToken): OutliningSpan[] { const elements: OutliningSpan[] = []; const collapseText = "..."; diff --git a/src/services/services.ts b/src/services/services.ts index 6cf50940fb..cef307eb8f 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -958,12 +958,7 @@ namespace ts { } class CancellationTokenObject implements CancellationToken { - throttleWaitMilliseconds?: number; - constructor(private cancellationToken: HostCancellationToken) { - if (cancellationToken.throttleWaitMilliseconds !== undefined) { - this.throttleWaitMilliseconds = cancellationToken.throttleWaitMilliseconds; - } } public isCancellationRequested() { @@ -977,42 +972,6 @@ namespace ts { } } - /** A cancellation that throttles calls to the host */ - export class ThrottledCancellationToken implements CancellationToken { - // Store when we last tried to cancel. Checking cancellation can be expensive (as we have - // to marshall over to the host layer). So we only bother actually checking once enough - // time has passed. - private lastCancellationCheckTime = 0; - // The minuimum duration to wait in milliseconds before querying host. - // The default of 10 milliseconds will be used unless throttleWaitMilliseconds - // is specified in the host cancellation token. - throttleWaitMilliseconds: number = 10; - - constructor(private hostCancellationToken: HostCancellationToken) { - if (hostCancellationToken.throttleWaitMilliseconds !== undefined) { - this.throttleWaitMilliseconds = hostCancellationToken.throttleWaitMilliseconds; - } - } - - public isCancellationRequested(): boolean { - const time = timestamp(); - const duration = Math.abs(time - this.lastCancellationCheckTime); - if (duration >= this.throttleWaitMilliseconds) { - // Check no more than the min wait in milliseconds - this.lastCancellationCheckTime = time; - return this.hostCancellationToken.isCancellationRequested(); - } - - return false; - } - - public throwIfCancellationRequested(): void { - if (this.isCancellationRequested()) { - throw new OperationCanceledException(); - } - } - } - export function createLanguageService(host: LanguageServiceHost, documentRegistry: DocumentRegistry = createDocumentRegistry(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(), host.getCurrentDirectory())): LanguageService { @@ -1025,7 +984,6 @@ namespace ts { const useCaseSensitivefileNames = host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(); const cancellationToken = new CancellationTokenObject(host.getCancellationToken && host.getCancellationToken()); - const throttledCancellationToken = new ThrottledCancellationToken(cancellationToken); const currentDirectory = host.getCurrentDirectory(); // Check if the localized messages json is set, otherwise query the host for it @@ -1583,11 +1541,11 @@ namespace ts { } function getNavigationBarItems(fileName: string): NavigationBarItem[] { - return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName), throttledCancellationToken); + return NavigationBar.getNavigationBarItems(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); } function getNavigationTree(fileName: string): NavigationTree { - return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName), throttledCancellationToken); + return NavigationBar.getNavigationTree(syntaxTreeCache.getCurrentSourceFile(fileName), cancellationToken); } function isTsOrTsxFile(fileName: string): boolean { @@ -1626,7 +1584,7 @@ namespace ts { function getOutliningSpans(fileName: string): OutliningSpan[] { // doesn't use compiler - no need to synchronize with host const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName); - return OutliningElementsCollector.collectElements(sourceFile, throttledCancellationToken); + return OutliningElementsCollector.collectElements(sourceFile, cancellationToken); } function getBraceMatchingAtPosition(fileName: string, position: number) { diff --git a/src/services/shims.ts b/src/services/shims.ts index de2fd39b8a..6fe9aed144 100644 --- a/src/services/shims.ts +++ b/src/services/shims.ts @@ -469,6 +469,29 @@ namespace ts { } } + /** A cancellation that throttles calls to the host */ + class ThrottledCancellationToken implements HostCancellationToken { + // Store when we last tried to cancel. Checking cancellation can be expensive (as we have + // to marshall over to the host layer). So we only bother actually checking once enough + // time has passed. + private lastCancellationCheckTime = 0; + + constructor(private hostCancellationToken: HostCancellationToken) { + } + + public isCancellationRequested(): boolean { + const time = timestamp(); + const duration = Math.abs(time - this.lastCancellationCheckTime); + if (duration > 10) { + // Check no more than once every 10 ms. + this.lastCancellationCheckTime = time; + return this.hostCancellationToken.isCancellationRequested(); + } + + return false; + } + } + export class CoreServicesShimHostAdapter implements ParseConfigHost, ModuleResolutionHost { public directoryExists: (directoryName: string) => boolean; diff --git a/src/services/types.ts b/src/services/types.ts index f9c33e15df..a6ca385259 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -121,7 +121,6 @@ namespace ts { export interface HostCancellationToken { isCancellationRequested(): boolean; - throttleWaitMilliseconds?: number; } // From 21ef9078adcdbee99c5ebf2041850ea7562cb55c Mon Sep 17 00:00:00 2001 From: Jason Ramsay Date: Thu, 23 Feb 2017 15:52:38 -0800 Subject: [PATCH 5/5] Wrapping LSHost's cancellationtoken with a throttle --- .../unittests/tsserverProjectSystem.ts | 7 +- src/server/editorServices.ts | 3 +- src/server/lsHost.ts | 1 + src/server/session.ts | 5 +- src/services/navigationBar.ts | 73 ++++++++++--------- src/services/services.ts | 30 ++++++++ src/services/shims.ts | 23 ------ 7 files changed, 80 insertions(+), 62 deletions(-) diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index d9abb0a907..353d0e2e18 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -176,11 +176,11 @@ namespace ts.projectSystem { } }; - export function createSession(host: server.ServerHost, typingsInstaller?: server.ITypingsInstaller, projectServiceEventHandler?: server.ProjectServiceEventHandler, cancellationToken?: server.ServerCancellationToken) { + export function createSession(host: server.ServerHost, typingsInstaller?: server.ITypingsInstaller, projectServiceEventHandler?: server.ProjectServiceEventHandler, cancellationToken?: server.ServerCancellationToken, throttleWaitMilliseconds?: number) { if (typingsInstaller === undefined) { typingsInstaller = new TestTypingsInstaller("/a/data/", /*throttleLimit*/5, host); } - return new TestSession(host, cancellationToken || server.nullCancellationToken, /*useSingleInferredProject*/ false, typingsInstaller, Utils.byteLength, process.hrtime, nullLogger, /*canUseEvents*/ projectServiceEventHandler !== undefined, projectServiceEventHandler); + return new TestSession(host, cancellationToken || server.nullCancellationToken, /*useSingleInferredProject*/ false, typingsInstaller, Utils.byteLength, process.hrtime, nullLogger, /*canUseEvents*/ projectServiceEventHandler !== undefined, projectServiceEventHandler, throttleWaitMilliseconds); } export interface CreateProjectServiceParameters { @@ -3320,6 +3320,7 @@ namespace ts.projectSystem { }, resetRequest: noop } + const session = createSession(host, /*typingsInstaller*/ undefined, /*projectServiceEventHandler*/ undefined, cancellationToken); expectedRequestId = session.getNextSeq(); @@ -3492,7 +3493,7 @@ namespace ts.projectSystem { }; const cancellationToken = new TestServerCancellationToken(/*cancelAfterRequest*/ 3); const host = createServerHost([f1, config]); - const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken); + const session = createSession(host, /*typingsInstaller*/ undefined, () => { }, cancellationToken, /*throttleWaitMilliseconds*/ 0); { session.executeCommandSeq({ command: "open", diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 36aa3939c8..4067d85fb3 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -270,7 +270,8 @@ namespace ts.server { public readonly cancellationToken: HostCancellationToken, public readonly useSingleInferredProject: boolean, readonly typingsInstaller: ITypingsInstaller = nullTypingsInstaller, - private readonly eventHandler?: ProjectServiceEventHandler) { + private readonly eventHandler?: ProjectServiceEventHandler, + public readonly throttleWaitMilliseconds?: number) { Debug.assert(!!host.createHash, "'ServerHost.createHash' is required for ProjectService"); diff --git a/src/server/lsHost.ts b/src/server/lsHost.ts index eee80d82e7..ec655c545e 100644 --- a/src/server/lsHost.ts +++ b/src/server/lsHost.ts @@ -16,6 +16,7 @@ namespace ts.server { readonly realpath?: (path: string) => string; constructor(private readonly host: ServerHost, private readonly project: Project, private readonly cancellationToken: HostCancellationToken) { + this.cancellationToken = new ThrottledCancellationToken(cancellationToken, project.projectService.throttleWaitMilliseconds); this.getCanonicalFileName = ts.createGetCanonicalFileName(this.host.useCaseSensitiveFileNames); if (host.trace) { diff --git a/src/server/session.ts b/src/server/session.ts index 262ba8da1d..e7b4fc4bd4 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -338,7 +338,8 @@ namespace ts.server { private hrtime: (start?: number[]) => number[], protected logger: Logger, protected readonly canUseEvents: boolean, - eventHandler?: ProjectServiceEventHandler) { + eventHandler?: ProjectServiceEventHandler, + private readonly throttleWaitMilliseconds?: number) { this.eventHander = canUseEvents ? eventHandler || (event => this.defaultEventHandler(event)) @@ -353,7 +354,7 @@ namespace ts.server { isCancellationRequested: () => cancellationToken.isCancellationRequested() } this.errorCheck = new MultistepOperation(multistepOperationHost); - this.projectService = new ProjectService(host, logger, cancellationToken, useSingleInferredProject, typingsInstaller, this.eventHander); + this.projectService = new ProjectService(host, logger, cancellationToken, useSingleInferredProject, typingsInstaller, this.eventHander, this.throttleWaitMilliseconds); this.gcTimer = new GcTimer(host, /*delay*/ 7000, logger); } diff --git a/src/services/navigationBar.ts b/src/services/navigationBar.ts index 811762fb73..7a2508fcfd 100644 --- a/src/services/navigationBar.ts +++ b/src/services/navigationBar.ts @@ -2,6 +2,36 @@ /* @internal */ namespace ts.NavigationBar { + /** + * Matches all whitespace characters in a string. Eg: + * + * "app. + * + * onactivated" + * + * matches because of the newline, whereas + * + * "app.onactivated" + * + * does not match. + */ + const whiteSpaceRegex = /\s+/g; + + // Keep sourceFile handy so we don't have to search for it every time we need to call `getText`. + let curCancellationToken: CancellationToken; + let curSourceFile: SourceFile; + + /** + * For performance, we keep navigation bar parents on a stack rather than passing them through each recursion. + * `parent` is the current parent and is *not* stored in parentsStack. + * `startNode` sets a new parent and `endNode` returns to the previous parent. + */ + let parentsStack: NavigationBarNode[] = []; + let parent: NavigationBarNode; + + // NavigationBarItem requires an array, but will not mutate it, so just give it this for performance. + let emptyChildItemArray: NavigationBarItem[] = []; + /** * Represents a navigation bar item and its children. * The returned NavigationBarItem is more complicated and doesn't include 'parent', so we use these to do work before converting. @@ -21,8 +51,7 @@ namespace ts.NavigationBar { return map(topLevelItems(rootNavigationBarNode(sourceFile)), convertToTopLevelItem); } finally { - curSourceFile = undefined; - curCancellationToken = undefined; + reset(); } } @@ -33,14 +62,18 @@ namespace ts.NavigationBar { return convertToTree(rootNavigationBarNode(sourceFile)); } finally { - curSourceFile = undefined; - curCancellationToken = undefined; + reset(); } } - // Keep sourceFile handy so we don't have to search for it every time we need to call `getText`. - let curCancellationToken: CancellationToken; - let curSourceFile: SourceFile; + function reset() { + curSourceFile = undefined; + curCancellationToken = undefined; + parentsStack = []; + parent = undefined; + emptyChildItemArray = []; + } + function nodeText(node: Node): string { return node.getText(curSourceFile); } @@ -58,14 +91,6 @@ namespace ts.NavigationBar { } } - /* - For performance, we keep navigation bar parents on a stack rather than passing them through each recursion. - `parent` is the current parent and is *not* stored in parentsStack. - `startNode` sets a new parent and `endNode` returns to the previous parent. - */ - const parentsStack: NavigationBarNode[] = []; - let parent: NavigationBarNode; - function rootNavigationBarNode(sourceFile: SourceFile): NavigationBarNode { Debug.assert(!parentsStack.length); const root: NavigationBarNode = { node: sourceFile, additionalNodes: undefined, parent: undefined, children: undefined, indent: 0 }; @@ -500,9 +525,6 @@ namespace ts.NavigationBar { } } - // NavigationBarItem requires an array, but will not mutate it, so just give it this for performance. - const emptyChildItemArray: NavigationBarItem[] = []; - function convertToTree(n: NavigationBarNode): NavigationTree { return { text: getItemName(n.node), @@ -623,19 +645,4 @@ namespace ts.NavigationBar { function isFunctionOrClassExpression(node: Node): boolean { return node.kind === SyntaxKind.FunctionExpression || node.kind === SyntaxKind.ArrowFunction || node.kind === SyntaxKind.ClassExpression; } - - /** - * Matches all whitespace characters in a string. Eg: - * - * "app. - * - * onactivated" - * - * matches because of the newline, whereas - * - * "app.onactivated" - * - * does not match. - */ - const whiteSpaceRegex = /\s+/g; } diff --git a/src/services/services.ts b/src/services/services.ts index cef307eb8f..d808ed28b7 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -972,6 +972,36 @@ namespace ts { } } + /* @internal */ + /** A cancellation that throttles calls to the host */ + export class ThrottledCancellationToken implements CancellationToken { + // Store when we last tried to cancel. Checking cancellation can be expensive (as we have + // to marshall over to the host layer). So we only bother actually checking once enough + // time has passed. + private lastCancellationCheckTime = 0; + + constructor(private hostCancellationToken: HostCancellationToken, private readonly throttleWaitMilliseconds = 20) { + } + + public isCancellationRequested(): boolean { + const time = timestamp(); + const duration = Math.abs(time - this.lastCancellationCheckTime); + if (duration >= this.throttleWaitMilliseconds) { + // Check no more than once every throttle wait milliseconds + this.lastCancellationCheckTime = time; + return this.hostCancellationToken.isCancellationRequested(); + } + + return false; + } + + public throwIfCancellationRequested(): void { + if (this.isCancellationRequested()) { + throw new OperationCanceledException(); + } + } + } + export function createLanguageService(host: LanguageServiceHost, documentRegistry: DocumentRegistry = createDocumentRegistry(host.useCaseSensitiveFileNames && host.useCaseSensitiveFileNames(), host.getCurrentDirectory())): LanguageService { diff --git a/src/services/shims.ts b/src/services/shims.ts index 6fe9aed144..de2fd39b8a 100644 --- a/src/services/shims.ts +++ b/src/services/shims.ts @@ -469,29 +469,6 @@ namespace ts { } } - /** A cancellation that throttles calls to the host */ - class ThrottledCancellationToken implements HostCancellationToken { - // Store when we last tried to cancel. Checking cancellation can be expensive (as we have - // to marshall over to the host layer). So we only bother actually checking once enough - // time has passed. - private lastCancellationCheckTime = 0; - - constructor(private hostCancellationToken: HostCancellationToken) { - } - - public isCancellationRequested(): boolean { - const time = timestamp(); - const duration = Math.abs(time - this.lastCancellationCheckTime); - if (duration > 10) { - // Check no more than once every 10 ms. - this.lastCancellationCheckTime = time; - return this.hostCancellationToken.isCancellationRequested(); - } - - return false; - } - } - export class CoreServicesShimHostAdapter implements ParseConfigHost, ModuleResolutionHost { public directoryExists: (directoryName: string) => boolean;