From 02d5cb045c07b727877d651f2f42785d13475286 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 2 Nov 2018 11:27:52 -0700 Subject: [PATCH 1/6] Expose file size from TextStorage Since it's for telemetry, it prefers to return a stale value rather than triggering file IO (though it will load the file if not even a stale value is available). --- src/server/scriptInfo.ts | 54 ++++++++++++-- src/testRunner/unittests/textStorage.ts | 95 ++++++++++++++++++++++--- 2 files changed, 133 insertions(+), 16 deletions(-) diff --git a/src/server/scriptInfo.ts b/src/server/scriptInfo.ts index 4e3a67748dd..9af99ea76ef 100644 --- a/src/server/scriptInfo.ts +++ b/src/server/scriptInfo.ts @@ -25,6 +25,14 @@ namespace ts.server { */ private lineMap: number[] | undefined; + /** + * When a large file is loaded, text will artificially be set to "". + * In order to be able to report correct telemetry, we store the actual + * file size in this case. (In other cases where text === "", e.g. + * for mixed content or dynamic files, fileSize will be undefined.) + */ + private fileSize: number | undefined; + /** * True if the text is for the file thats open in the editor */ @@ -56,10 +64,11 @@ namespace ts.server { this.switchToScriptVersionCache(); } - public useText(newText?: string) { + public useText_TestOnly(newText?: string) { this.svc = undefined; this.text = newText; this.lineMap = undefined; + this.fileSize = undefined; this.version.text++; } @@ -68,29 +77,41 @@ namespace ts.server { this.ownFileText = false; this.text = undefined; this.lineMap = undefined; + this.fileSize = undefined; } /** * Set the contents as newText * returns true if text changed */ - public reload(newText: string) { + public reload(newText: string): boolean { Debug.assert(newText !== undefined); // Reload always has fresh content this.pendingReloadFromDisk = false; + // We only need both this.text and this.fileSize if this.text + // is artificially empty because it was too large. + // We assume that an empty string passed to reload is not + // for a file that was too large to store. + this.fileSize = undefined; + // If text changed set the text // This also ensures that if we had switched to version cache, // we are switching back to text. // The change to version cache will happen when needed // Thus avoiding the computation if there are no changes if (this.text !== newText) { - this.useText(newText); + this.text = newText; + this.version.text++; + this.svc = undefined; + this.lineMap = undefined; // We cant guarantee new text is own file text this.ownFileText = false; return true; } + + return false; } /** @@ -98,7 +119,9 @@ namespace ts.server { * returns true if text changed */ public reloadWithFileText(tempFileName?: string) { - const reloaded = this.reload(this.getFileText(tempFileName)); + const { text: newText, fileSize } = this.getFileTextAndSize(tempFileName); + const reloaded = this.reload(newText); + this.fileSize = fileSize; // NB: after reload since reload clears it this.ownFileText = !tempFileName || tempFileName === this.fileName; return reloaded; } @@ -118,6 +141,23 @@ namespace ts.server { this.pendingReloadFromDisk = true; } + /** + * For telemetry purposes, we would like to be able to report the size of the file. + * However, we do not want telemetry to require extra file I/O so we report a size + * that may be stale (e.g. may not reflect change made on disk since the last reload). + * NB: Will read from disk if the file contents have never been loaded because + * telemetry falsely indicating size 0 would be counter-productive. + */ + public getTelemetryFileSize(): number { + return !!this.fileSize + ? this.fileSize + : !!this.text // Check text before svc because its length is cheaper + ? this.text.length // Could be wrong if this.pendingReloadFromDisk + : !!this.svc + ? this.svc.getSnapshot().getLength() // Could be wrong if this.pendingReloadFromDisk + : this.getSnapshot().getLength(); // Should be strictly correct + } + public getSnapshot(): IScriptSnapshot { return this.useScriptVersionCacheIfValidOrOpen() ? this.svc!.getSnapshot() @@ -161,7 +201,7 @@ namespace ts.server { return this.svc!.positionToLineOffset(position); } - private getFileText(tempFileName?: string) { + private getFileTextAndSize(tempFileName?: string): { text: string, fileSize?: number } { let text: string; const fileName = tempFileName || this.fileName; const getText = () => text === undefined ? (text = this.host.readFile(fileName) || "") : text; @@ -173,10 +213,10 @@ namespace ts.server { const service = this.info.containingProjects[0].projectService; service.logger.info(`Skipped loading contents of large file ${fileName} for info ${this.info.fileName}: fileSize: ${fileSize}`); this.info.containingProjects[0].projectService.sendLargeFileReferencedEvent(fileName, fileSize); - return ""; + return { text: "", fileSize }; } } - return getText(); + return { text: getText() }; } private switchToScriptVersionCache(): ScriptVersionCache { diff --git a/src/testRunner/unittests/textStorage.ts b/src/testRunner/unittests/textStorage.ts index 5486cf57745..b57d1109c7b 100644 --- a/src/testRunner/unittests/textStorage.ts +++ b/src/testRunner/unittests/textStorage.ts @@ -18,7 +18,7 @@ namespace ts.textStorage { const ts2 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); ts1.useScriptVersionCache_TestOnly(); - ts2.useText(); + ts2.useText_TestOnly(); const lineMap = computeLineStarts(f.content); @@ -29,20 +29,20 @@ namespace ts.textStorage { for (let offset = 0; offset < end - start; offset++) { const pos1 = ts1.lineOffsetToPosition(line + 1, offset + 1); const pos2 = ts2.lineOffsetToPosition(line + 1, offset + 1); - assert.isTrue(pos1 === pos2, `lineOffsetToPosition ${line + 1}-${offset + 1}: expected ${pos1} to equal ${pos2}`); + assert.strictEqual(pos1, pos2, `lineOffsetToPosition ${line + 1}-${offset + 1}: expected ${pos1} to equal ${pos2}`); } const {start: start1, length: length1 } = ts1.lineToTextSpan(line); const {start: start2, length: length2 } = ts2.lineToTextSpan(line); - assert.isTrue(start1 === start2, `lineToTextSpan ${line}::start:: expected ${start1} to equal ${start2}`); - assert.isTrue(length1 === length2, `lineToTextSpan ${line}::length:: expected ${length1} to equal ${length2}`); + assert.strictEqual(start1, start2, `lineToTextSpan ${line}::start:: expected ${start1} to equal ${start2}`); + assert.strictEqual(length1, length2, `lineToTextSpan ${line}::length:: expected ${length1} to equal ${length2}`); } for (let pos = 0; pos < f.content.length; pos++) { const { line: line1, offset: offset1 } = ts1.positionToLineOffset(pos); const { line: line2, offset: offset2 } = ts2.positionToLineOffset(pos); - assert.isTrue(line1 === line2, `positionToLineOffset ${pos}::line:: expected ${line1} to equal ${line2}`); - assert.isTrue(offset1 === offset2, `positionToLineOffset ${pos}::offset:: expected ${offset1} to equal ${offset2}`); + assert.strictEqual(line1, line2, `positionToLineOffset ${pos}::line:: expected ${line1} to equal ${line2}`); + assert.strictEqual(offset1, offset2, `positionToLineOffset ${pos}::offset:: expected ${offset1} to equal ${offset2}`); } }); @@ -52,16 +52,93 @@ namespace ts.textStorage { const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); ts1.getSnapshot(); - assert.isTrue(!ts1.hasScriptVersionCache_TestOnly(), "should not have script version cache - 1"); + assert.isFalse(ts1.hasScriptVersionCache_TestOnly(), "should not have script version cache - 1"); ts1.edit(0, 5, " "); assert.isTrue(ts1.hasScriptVersionCache_TestOnly(), "have script version cache - 1"); - ts1.useText(); - assert.isTrue(!ts1.hasScriptVersionCache_TestOnly(), "should not have script version cache - 2"); + ts1.useText_TestOnly(); + assert.isFalse(ts1.hasScriptVersionCache_TestOnly(), "should not have script version cache - 2"); ts1.getLineInfo(0); assert.isTrue(ts1.hasScriptVersionCache_TestOnly(), "have script version cache - 2"); }); + + it("should be able to return the file size immediately after construction", () => { + const host = projectSystem.createServerHost([f]); + // Since script info is not used in these tests, just cheat by passing undefined + const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); + + assert.strictEqual(f.content.length, ts1.getTelemetryFileSize()); + }); + + it("should be able to return the file size when backed by text", () => { + const host = projectSystem.createServerHost([f]); + // Since script info is not used in these tests, just cheat by passing undefined + const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); + + ts1.useText_TestOnly(f.content); + assert.isFalse(ts1.hasScriptVersionCache_TestOnly()); + + assert.strictEqual(f.content.length, ts1.getTelemetryFileSize()); + }); + + it("should be able to return the file size when backed by a script version cache", () => { + const host = projectSystem.createServerHost([f]); + // Since script info is not used in these tests, just cheat by passing undefined + const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); + + ts1.useScriptVersionCache_TestOnly(); + assert.isTrue(ts1.hasScriptVersionCache_TestOnly()); + + assert.strictEqual(f.content.length, ts1.getTelemetryFileSize()); + }); + + it("should be able to return the file size when a JS file is too large to load into text", () => { + const largeFile = { + path: "/a/large.js", + content: " ".repeat(server.maxFileSize + 1) + }; + + const host = projectSystem.createServerHost([largeFile]); + + // The large-file handling requires a ScriptInfo with a containing project + const projectService = projectSystem.createProjectService(host); + projectService.openClientFile(largeFile.path); + const scriptInfo = projectService.getScriptInfo(largeFile.path); + + const ts1 = new server.TextStorage(host, server.asNormalizedPath(largeFile.path), /*initialVersion*/ undefined, scriptInfo!); + + assert.isTrue(ts1.reloadFromDisk()); + assert.isFalse(ts1.hasScriptVersionCache_TestOnly()); + + assert.strictEqual(largeFile.content.length, ts1.getTelemetryFileSize()); + }); + + it("should return the file size without reloading the file", () => { + const oldText = "hello"; + const newText = "goodbye"; + + const changingFile = { + path: "/a/changing.ts", + content: oldText + }; + + const host = projectSystem.createServerHost([changingFile]); + // Since script info is not used in these tests, just cheat by passing undefined + const ts1 = new server.TextStorage(host, server.asNormalizedPath(changingFile.path), /*initialVersion*/ undefined, /*info*/undefined!); + + assert.isTrue(ts1.reloadFromDisk()); + + // Refresh the file and notify TextStorage + host.writeFile(changingFile.path, newText); + ts1.delayReloadFromFileIntoText(); + + assert.strictEqual(oldText.length, ts1.getTelemetryFileSize()); + + assert.isTrue(ts1.reloadWithFileText()); + + assert.strictEqual(newText.length, ts1.getTelemetryFileSize()); + }); }); } From 2c7d67fdedf05ba93fdd3eee67e4cd1ef904e193 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 15 Nov 2018 10:54:48 -0800 Subject: [PATCH 2/6] Expose aggregate file sizes in FileStats We're not sure that file counts are a good proxy for project size and this will give us more direct insight. --- src/server/editorServices.ts | 13 ++++++++++++- src/server/project.ts | 27 ++++++++++++++++++++++----- src/server/scriptInfo.ts | 5 +++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index ac80f43c84c..23baa9119bf 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -123,11 +123,22 @@ namespace ts.server { export interface FileStats { readonly js: number; + readonly jsSize?: number; + readonly jsx: number; + readonly jsxSize?: number; + readonly ts: number; + readonly tsSize?: number; + readonly tsx: number; + readonly tsxSize?: number; + readonly dts: number; + readonly dtsSize?: number; + readonly deferred: number; + readonly deferredSize?: number; } export interface OpenFileInfo { @@ -1600,7 +1611,7 @@ namespace ts.server { setProjectOptionsUsed(project); const data: ProjectInfoTelemetryEventData = { projectId: this.host.createSHA256Hash(project.projectName), - fileStats: countEachFileTypes(project.getScriptInfos()), + fileStats: countEachFileTypes(project.getScriptInfos(), /*includeSizes*/ true), compilerOptions: convertCompilerOptionsForTelemetry(project.getCompilationSettings()), typeAcquisition: convertTypeAcquisition(project.getTypeAcquisition()), extends: projectOptions && projectOptions.configHasExtendsProperty, diff --git a/src/server/project.ts b/src/server/project.ts index 7b11f48fbd6..dbfa9100ca4 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -10,26 +10,43 @@ namespace ts.server { export type Mutable = { -readonly [K in keyof T]: T[K]; }; /* @internal */ - export function countEachFileTypes(infos: ScriptInfo[]): FileStats { - const result: Mutable = { js: 0, jsx: 0, ts: 0, tsx: 0, dts: 0, deferred: 0 }; + export function countEachFileTypes(infos: ScriptInfo[], includeSizes = false): FileStats { + const result: Mutable = { + js: 0, jsSize: 0, + jsx: 0, jsxSize: 0, + ts: 0, tsSize: 0, + tsx: 0, tsxSize: 0, + dts: 0, dtsSize: 0, + deferred: 0, deferredSize: 0, + }; for (const info of infos) { + const fileSize = includeSizes ? info.getTelemetryFileSize() : 0; switch (info.scriptKind) { case ScriptKind.JS: result.js += 1; + result.jsSize! += fileSize; break; case ScriptKind.JSX: result.jsx += 1; + result.jsxSize! += fileSize; break; case ScriptKind.TS: - fileExtensionIs(info.fileName, Extension.Dts) - ? result.dts += 1 - : result.ts += 1; + if (fileExtensionIs(info.fileName, Extension.Dts)) { + result.dts += 1; + result.dtsSize! += fileSize; + } + else { + result.ts += 1; + result.tsSize! += fileSize; + } break; case ScriptKind.TSX: result.tsx += 1; + result.tsxSize! += fileSize; break; case ScriptKind.Deferred: result.deferred += 1; + result.deferredSize! += fileSize; break; } } diff --git a/src/server/scriptInfo.ts b/src/server/scriptInfo.ts index 9af99ea76ef..defc28f7495 100644 --- a/src/server/scriptInfo.ts +++ b/src/server/scriptInfo.ts @@ -316,6 +316,11 @@ namespace ts.server { return this.textStorage.version; } + /*@internal*/ + getTelemetryFileSize() { + return this.textStorage.getTelemetryFileSize(); + } + /*@internal*/ public isDynamicOrHasMixedContent() { return this.hasMixedContent || this.isDynamic; From d5b95b5904615c75fe32a03910e69f5d1872161f Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 15 Nov 2018 15:54:21 -0800 Subject: [PATCH 3/6] Update API baseline --- tests/baselines/reference/api/tsserverlibrary.d.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 120f3bcb5a1..a6ac75b4dca 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -8437,11 +8437,17 @@ declare namespace ts.server { } interface FileStats { readonly js: number; + readonly jsSize?: number; readonly jsx: number; + readonly jsxSize?: number; readonly ts: number; + readonly tsSize?: number; readonly tsx: number; + readonly tsxSize?: number; readonly dts: number; + readonly dtsSize?: number; readonly deferred: number; + readonly deferredSize?: number; } interface OpenFileInfo { readonly checkJs: boolean; From 8bfdb4145da1c1a00285e9d87063f9485af32e6c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 15 Nov 2018 16:00:35 -0800 Subject: [PATCH 4/6] Fix tests and lint --- src/server/project.ts | 2 +- src/testRunner/unittests/telemetry.ts | 5 +++-- src/testRunner/unittests/tsserverProjectSystem.ts | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/server/project.ts b/src/server/project.ts index dbfa9100ca4..45d932abc3e 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -11,7 +11,7 @@ namespace ts.server { /* @internal */ export function countEachFileTypes(infos: ScriptInfo[], includeSizes = false): FileStats { - const result: Mutable = { + const result: Mutable = { js: 0, jsSize: 0, jsx: 0, jsxSize: 0, ts: 0, tsSize: 0, diff --git a/src/testRunner/unittests/telemetry.ts b/src/testRunner/unittests/telemetry.ts index 1c11a40c2ee..2eee552b38f 100644 --- a/src/testRunner/unittests/telemetry.ts +++ b/src/testRunner/unittests/telemetry.ts @@ -215,11 +215,12 @@ namespace ts.projectSystem { const file = makeFile("/a.js"); const tsconfig = makeFile("/jsconfig.json", {}); const et = new TestServerEventManager([tsconfig, file]); - et.host.getFileSize = () => server.maxProgramSizeForNonTsFiles + 1; + const fileSize = server.maxProgramSizeForNonTsFiles + 1; + et.host.getFileSize = () => fileSize; et.service.openClientFile(file.path); et.getEvent(server.ProjectLanguageServiceStateEvent); et.assertProjectInfoTelemetryEvent({ - fileStats: fileStats({ js: 1 }), + fileStats: fileStats({ js: 1, jsSize: fileSize }), compilerOptions: autoJsCompilerOptions, configFileName: "jsconfig.json", typeAcquisition: { diff --git a/src/testRunner/unittests/tsserverProjectSystem.ts b/src/testRunner/unittests/tsserverProjectSystem.ts index b292ec5486c..709f48b1d56 100644 --- a/src/testRunner/unittests/tsserverProjectSystem.ts +++ b/src/testRunner/unittests/tsserverProjectSystem.ts @@ -183,7 +183,7 @@ namespace ts.projectSystem { } export function fileStats(nonZeroStats: Partial): server.FileStats { - return { ts: 0, tsx: 0, dts: 0, js: 0, jsx: 0, deferred: 0, ...nonZeroStats }; + return { ts: 0, tsSize: 0, tsx: 0, tsxSize: 0, dts: 0, dtsSize: 0, js: 0, jsSize: 0, jsx: 0, jsxSize: 0, deferred: 0, deferredSize: 0, ...nonZeroStats }; } export interface ConfigFileDiagnostic { From a8c634d70db09137ce5e7ef5b77d2ff3e80cf99d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 16 Nov 2018 10:43:15 -0800 Subject: [PATCH 5/6] Restore useText --- src/server/scriptInfo.ts | 14 +++----------- src/testRunner/unittests/textStorage.ts | 6 +++--- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/server/scriptInfo.ts b/src/server/scriptInfo.ts index defc28f7495..77329099871 100644 --- a/src/server/scriptInfo.ts +++ b/src/server/scriptInfo.ts @@ -64,7 +64,8 @@ namespace ts.server { this.switchToScriptVersionCache(); } - public useText_TestOnly(newText?: string) { + /** Public for testing */ + public useText(newText?: string) { this.svc = undefined; this.text = newText; this.lineMap = undefined; @@ -90,22 +91,13 @@ namespace ts.server { // Reload always has fresh content this.pendingReloadFromDisk = false; - // We only need both this.text and this.fileSize if this.text - // is artificially empty because it was too large. - // We assume that an empty string passed to reload is not - // for a file that was too large to store. - this.fileSize = undefined; - // If text changed set the text // This also ensures that if we had switched to version cache, // we are switching back to text. // The change to version cache will happen when needed // Thus avoiding the computation if there are no changes if (this.text !== newText) { - this.text = newText; - this.version.text++; - this.svc = undefined; - this.lineMap = undefined; + this.useText(newText); // We cant guarantee new text is own file text this.ownFileText = false; return true; diff --git a/src/testRunner/unittests/textStorage.ts b/src/testRunner/unittests/textStorage.ts index b57d1109c7b..5dce083120b 100644 --- a/src/testRunner/unittests/textStorage.ts +++ b/src/testRunner/unittests/textStorage.ts @@ -18,7 +18,7 @@ namespace ts.textStorage { const ts2 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); ts1.useScriptVersionCache_TestOnly(); - ts2.useText_TestOnly(); + ts2.useText(); const lineMap = computeLineStarts(f.content); @@ -57,7 +57,7 @@ namespace ts.textStorage { ts1.edit(0, 5, " "); assert.isTrue(ts1.hasScriptVersionCache_TestOnly(), "have script version cache - 1"); - ts1.useText_TestOnly(); + ts1.useText(); assert.isFalse(ts1.hasScriptVersionCache_TestOnly(), "should not have script version cache - 2"); ts1.getLineInfo(0); @@ -77,7 +77,7 @@ namespace ts.textStorage { // Since script info is not used in these tests, just cheat by passing undefined const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path), /*initialVersion*/ undefined, /*info*/undefined!); - ts1.useText_TestOnly(f.content); + ts1.useText(f.content); assert.isFalse(ts1.hasScriptVersionCache_TestOnly()); assert.strictEqual(f.content.length, ts1.getTelemetryFileSize()); From 5c1c34a16e4326569b0708c9b11a354f7c848f7c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 16 Nov 2018 11:00:41 -0800 Subject: [PATCH 6/6] Add telemetry test --- src/testRunner/unittests/telemetry.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/testRunner/unittests/telemetry.ts b/src/testRunner/unittests/telemetry.ts index 2eee552b38f..f2a7d854124 100644 --- a/src/testRunner/unittests/telemetry.ts +++ b/src/testRunner/unittests/telemetry.ts @@ -211,6 +211,26 @@ namespace ts.projectSystem { }, "/jsconfig.json"); }); + it("sends telemetry for file sizes", () => { + const jsFile = makeFile("/a.js", "1"); + const tsFile = makeFile("/b.ts", "12"); + const tsconfig = makeFile("/jsconfig.json", { + compilerOptions: autoJsCompilerOptions + }); + const et = new TestServerEventManager([tsconfig, jsFile, tsFile]); + et.service.openClientFile(jsFile.path); + et.assertProjectInfoTelemetryEvent({ + fileStats: fileStats({ js: 1, jsSize: 1, ts: 1, tsSize: 2 }), + compilerOptions: autoJsCompilerOptions, + typeAcquisition: { + enable: true, + include: false, + exclude: false, + }, + configFileName: "jsconfig.json", + }, "/jsconfig.json"); + }); + it("detects whether language service was disabled", () => { const file = makeFile("/a.js"); const tsconfig = makeFile("/jsconfig.json", {});