Add debounce to import tracking (microsoft#4875)

rchiodo · web-flow · commit 4e7df1eada2f · 2019-03-21T16:19:54.000-07:00
* Add debounce to import tracking
Add steps on how to profile
Skip having test-results.xml checked in.

* Add news entry

* Update casing

* Move profiling to wiki
Use lineAt instead of getText.splitLines

* Update news entry

* Remove debounce changes
Implement per file timeout instead

* Use isemptyOrWhitespace
Fix timeout to be when finally done typing
diff --git a/.gitignore b/.gitignore
@@ -28,4 +28,5 @@ obj/**
 tmp/**
 .python-version
 .vs/
+test-results.xml
 !build/
diff --git a/images/JavascriptProfiler.png b/images/JavascriptProfiler.png
diff --git a/news/3 Code Health/4852.md b/news/3 Code Health/4852.md
@@ -1 +1 @@
-Hash imports so we can get every import and not just known ones. We have to unhash them on the other side to determine if a specific package or not.
+Hash imports so we can get every import and not just known ones. Should happen at most once every 5 seconds.
diff --git a/src/client/telemetry/importTracker.ts b/src/client/telemetry/importTracker.ts
@@ -8,8 +8,9 @@ import * as path from 'path';
 import { TextDocument } from 'vscode';
 
 import { sendTelemetryEvent } from '.';
-import { noop, sleep } from '../../test/core';
+import { noop } from '../../test/core';
 import { IDocumentManager } from '../common/application/types';
+import { isTestExecution } from '../common/constants';
 import { IHistoryProvider } from '../datascience/types';
 import { ICodeExecutionManager } from '../terminals/types';
 import { EventName } from './constants';
@@ -18,9 +19,14 @@ import { IImportTracker } from './types';
 const ImportRegEx = /^(?!['"#]).*from\s+([a-zA-Z0-9_\.]+)\s+import.*(?!['"])|^(?!['"#]).*import\s+([a-zA-Z0-9_\., ]+).*(?!['"])/;
 const MAX_DOCUMENT_LINES = 1000;
 
+// Capture isTestExecution on module load so that a test can turn it off and still
+// have this value set.
+const testExecution = isTestExecution();
+
 @injectable()
 export class ImportTracker implements IImportTracker {
 
+    private pendingDocs = new Map<string, NodeJS.Timer>();
     private sentMatches: Set<string> = new Set<string>();
     // tslint:disable-next-line:no-require-imports
     private hashFn = require('hash.js').sha256;
@@ -44,21 +50,51 @@ export class ImportTracker implements IImportTracker {
     }
 
     public async activate(): Promise<void> {
-        // Act like all of our open documents just opened. Don't do this now though. We don't want
-        // to hold up the activate.
-        await sleep(1000);
+        // Act like all of our open documents just opened. Our timeout will make sure this is delayed
         this.documentManager.textDocuments.forEach(d => this.onOpenedOrSavedDocument(d));
     }
 
+    private getDocumentLines(document: TextDocument) : string [] {
+        return Array.apply(null, {length: Math.min(document.lineCount, MAX_DOCUMENT_LINES)}).map((a, i) => {
+            const line = document.lineAt(i);
+            if (line && !line.isEmptyOrWhitespace) {
+                return line.text;
+            }
+            return undefined;
+        }).filter((f: string | undefined) => f);
+    }
+
     private onOpenedOrSavedDocument(document: TextDocument) {
         // Make sure this is a python file.
         if (path.extname(document.fileName) === '.py') {
             // Parse the contents of the document, looking for import matches on each line
-            const lines = document.getText().splitLines({ trim: true, removeEmptyEntries: true });
-            this.lookForImports(lines.slice(0, Math.min(lines.length, MAX_DOCUMENT_LINES)), EventName.KNOWN_IMPORT_FROM_FILE);
+            this.scheduleDocument(document);
         }
     }
 
+    private scheduleDocument(document: TextDocument) {
+        // If already scheduled, cancel.
+        if (this.pendingDocs.has(document.fileName)) {
+            clearTimeout(this.pendingDocs.get(document.fileName));
+            this.pendingDocs.delete(document.fileName);
+        }
+
+        // Now schedule a new one.
+        if (testExecution) {
+            // During a test, check right away. It needs to be synchronous.
+            this.checkDocument(document);
+        } else {
+            // Wait five seconds to make sure we don't already have this document pending.
+            this.pendingDocs.set(document.fileName, setTimeout(() => this.checkDocument(document), 5000));
+        }
+    }
+
+    private checkDocument(document: TextDocument) {
+        this.pendingDocs.delete(document.fileName);
+        const lines = this.getDocumentLines(document);
+        this.lookForImports(lines, EventName.KNOWN_IMPORT_FROM_FILE);
+    }
+
     private onExecutedCode(code: string) {
         const lines = code.splitLines({ trim: true, removeEmptyEntries: true });
         this.lookForImports(lines, EventName.KNOWN_IMPORT_FROM_EXECUTION);
diff --git a/src/test/datascience/editor-integration/helpers.ts b/src/test/datascience/editor-integration/helpers.ts
@@ -29,6 +29,7 @@ export function createDocument(inputText: string, fileName: string, fileVersion:
         const testRange = new Range(index, 0, index, line.length);
         textLine.setup(l => l.text).returns(() => line);
         textLine.setup(l => l.range).returns(() => testRange);
+        textLine.setup(l => l.isEmptyOrWhitespace).returns(() => line.trim().length === 0);
         return textLine;
     });
     document.setup(d => d.lineAt(TypeMoq.It.isAnyNumber())).returns((index: number) => textLines[index].object);

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-Hash imports so we can get every import and not just known ones. We have to unhash them on the other side to determine if a specific package or not.`
	`1`	`+Hash imports so we can get every import and not just known ones. Should happen at most once every 5 seconds.`