From 3c5160ce509d45d442790abda8a22faa5c41dbc7 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Mon, 20 Dec 2021 01:49:45 -0500 Subject: [PATCH] Improves open editor repo locator perf Reduces startup git calls --- src/git/gitProvider.ts | 7 +- src/git/gitProviderService.ts | 233 ++++++++++++++++++---------------- src/git/providers/localGitProvider.ts | 88 +++++++------ src/system/path.ts | 14 +- src/system/promise.ts | 6 +- 5 files changed, 181 insertions(+), 167 deletions(-) diff --git a/src/git/gitProvider.ts b/src/git/gitProvider.ts index 8150c17..6595df5 100644 --- a/src/git/gitProvider.ts +++ b/src/git/gitProvider.ts @@ -344,7 +344,7 @@ export interface GitProvider { // options?: { ref?: string | undefined }, // ): Promise; - getRepoPath(filePath: string, isDirectory: boolean): Promise; + getRepoPath(filePath: string, isDirectory?: boolean): Promise; // getRepoPathOrActive(uri: Uri | undefined, editor: TextEditor | undefined): Promise; // getRepositories(predicate?: (repo: Repository) => boolean): Promise>; @@ -397,11 +397,6 @@ export interface GitProvider { isActiveRepoPath(repoPath: string | undefined, editor?: TextEditor): Promise; isTrackable(uri: Uri): boolean; - isTracked( - fileNameOrUri: string | GitUri, - repoPath?: string, - options?: { ref?: string | undefined; skipCacheUpdate?: boolean | undefined }, - ): Promise; getDiffTool(repoPath?: string): Promise; openDiffTool( diff --git a/src/git/gitProviderService.ts b/src/git/gitProviderService.ts index 07e1cec..b2493c4 100644 --- a/src/git/gitProviderService.ts +++ b/src/git/gitProviderService.ts @@ -28,6 +28,7 @@ import { import { Container } from '../container'; import { Logger } from '../logger'; import { Arrays, debug, gate, Iterables, log, Paths, Promises, Strings } from '../system'; +import { PromiseOrValue } from '../system/promise'; import { vslsUriPrefixRegex } from '../vsls/vsls'; import { ProviderNotFoundError } from './errors'; import { @@ -105,6 +106,20 @@ export class GitProviderService implements Disposable { private fireProvidersChanged(added?: GitProvider[], removed?: GitProvider[]) { this._etag = Date.now(); + if (this._pathToRepoPathCache.size !== 0) { + if (removed?.length) { + // If a repository was removed, clear the cache for all paths + this._pathToRepoPathCache.clear(); + } else if (added?.length) { + // If a provider was added, only preserve paths with a resolved repoPath + for (const [key, value] of this._pathToRepoPathCache) { + if (value === null || Promises.is(value)) { + this._pathToRepoPathCache.delete(key); + } + } + } + } + this._onDidChangeProviders.fire({ added: added ?? [], removed: removed ?? [] }); } @@ -115,6 +130,20 @@ export class GitProviderService implements Disposable { private fireRepositoriesChanged(added?: Repository[], removed?: Repository[]) { this._etag = Date.now(); + if (this._pathToRepoPathCache.size !== 0) { + if (removed?.length) { + // If a repository was removed, clear the cache for all paths + this._pathToRepoPathCache.clear(); + } else if (added?.length) { + // If a repository was added, only preserve paths with a resolved repoPath + for (const [key, value] of this._pathToRepoPathCache) { + if (value === null || Promises.is(value)) { + this._pathToRepoPathCache.delete(key); + } + } + } + } + this._onDidChangeRepositories.fire({ added: added ?? [], removed: removed ?? [] }); } @@ -193,7 +222,7 @@ export class GitProviderService implements Disposable { configuration.getAny( BuiltInGitConfiguration.AutoRepositoryDetection, ) ?? true; - if (autoRepositoryDetection !== false) { + if (autoRepositoryDetection !== false && autoRepositoryDetection !== 'openEditors') { void this.discoverRepositories(e.added); } } @@ -273,7 +302,7 @@ export class GitProviderService implements Disposable { // } getCachedRepository(repoPath: string): Repository | undefined { - return this._repositories.get(repoPath); + return repoPath && this._repositories.size !== 0 ? this._repositories.get(repoPath) : undefined; } /** @@ -367,7 +396,7 @@ export class GitProviderService implements Disposable { BuiltInGitConfiguration.AutoRepositoryDetection, ) ?? true; - if (autoRepositoryDetection !== false) { + if (autoRepositoryDetection !== false && autoRepositoryDetection !== 'openEditors') { void this.discoverRepositories(workspaceFolders); return; @@ -1371,66 +1400,25 @@ export class GitProviderService implements Disposable { return provider.getRemotesCore(path, providers, options); } - async getRepoPath(filePath: string, options?: { ref?: string }): Promise; - async getRepoPath(uri: Uri | undefined, options?: { ref?: string }): Promise; + async getRepoPath(filePath: string): Promise; + async getRepoPath(uri: Uri | undefined): Promise; @log({ exit: path => `returned ${path}` }) - async getRepoPath( - filePathOrUri: string | Uri | undefined, - options?: { ref?: string }, - ): Promise { + async getRepoPath(filePathOrUri: string | Uri | undefined): Promise { if (filePathOrUri == null) return this.highlanderRepoPath; if (GitUri.is(filePathOrUri)) return filePathOrUri.repoPath; - const cc = Logger.getCorrelationContext(); + // const autoRepositoryDetection = + // configuration.getAny( + // BuiltInGitConfiguration.AutoRepositoryDetection, + // ) ?? true; - // Don't save the tracking info to the cache, because we could be looking in the wrong place (e.g. looking in the root when the file is in a submodule) - let repo = await this.getRepository(filePathOrUri, { ...options, skipCacheUpdate: true }); - if (repo != null) return repo.path; - - const { provider, path } = this.getProvider(filePathOrUri); - const rp = await provider.getRepoPath(path, false); - - // const rp = await this.getRepoPathCore( - // typeof filePathOrUri === 'string' ? filePathOrUri : filePathOrUri.fsPath, - // false, + // const repo = await this.getRepository( + // filePathOrUri, + // autoRepositoryDetection === true || autoRepositoryDetection === 'openEditors', // ); - if (rp == null) return undefined; - - // Recheck this._repositoryTree.get(rp) to make sure we haven't already tried adding this due to awaits - if (this._repositories.get(rp) != null) return rp; - const isVslsScheme = - typeof filePathOrUri === 'string' ? undefined : filePathOrUri.scheme === DocumentSchemes.Vsls; - - // If this new repo is inside one of our known roots and we we don't already know about, add it - const root = this.findRepositoryForPath(rp, isVslsScheme); - - let folder; - if (root != null) { - // Not sure why I added this for vsls (I can't see a reason for it anymore), but if it is added it will break submodules - // rp = root.path; - folder = root.folder; - } else { - folder = workspace.getWorkspaceFolder(GitUri.file(rp, isVslsScheme)); - if (folder == null) { - const parts = rp.split(slash); - folder = { - uri: GitUri.file(rp, isVslsScheme), - name: parts[parts.length - 1], - index: this.container.git.repositoryCount, - }; - } - } - - Logger.log(cc, `Repository found in '${rp}'`); - repo = provider.createRepository(folder, rp, false); - this._repositories.set(rp, repo); - - void this.updateContext(); - // Send a notification that the repositories changed - queueMicrotask(() => this.fireRepositoriesChanged([repo!])); - - return rp; + const repo = await this.getRepository(filePathOrUri, true); + return repo?.path; } @log({ @@ -1443,60 +1431,102 @@ export class GitProviderService implements Disposable { return this.getActiveRepoPath(editor); } - async getRepository( - repoPath: string, - options?: { ref?: string; skipCacheUpdate?: boolean }, - ): Promise; - async getRepository( - uri: Uri, - options?: { ref?: string; skipCacheUpdate?: boolean }, - ): Promise; - async getRepository( - repoPathOrUri: string | Uri, - options?: { ref?: string; skipCacheUpdate?: boolean }, - ): Promise; - @log({ - exit: repo => `returned ${repo != null ? `${repo.path}` : 'undefined'}`, - }) - async getRepository( - repoPathOrUri: string | Uri, - options: { ref?: string; skipCacheUpdate?: boolean } = {}, - ): Promise { - let isVslsScheme; + private _pathToRepoPathCache = new Map>(); - let path: string; - if (typeof repoPathOrUri === 'string') { - const repo = this._repositories.get(repoPathOrUri); - if (repo != null) return repo; + async getRepository(repoPath: string, createIfNeeded?: boolean): Promise; + async getRepository(uri: Uri, createIfNeeded?: boolean): Promise; + async getRepository(repoPathOrUri: string | Uri, createIfNeeded?: boolean): Promise; + @log({ exit: repo => `returned ${repo?.path ?? 'undefined'}` }) + async getRepository(repoPathOrUri: string | Uri, createIfNeeded: boolean = false): Promise { + if (!createIfNeeded && this.repositoryCount === 0) return undefined; - path = repoPathOrUri; - isVslsScheme = undefined; + const cc = Logger.getCorrelationContext(); + + let isVslsScheme: boolean | undefined; + let repo: Repository | undefined; + let rp: string | null; + + let filePath: string; + if (typeof repoPathOrUri === 'string') { + filePath = Strings.normalizePath(repoPathOrUri); } else { - if (GitUri.is(repoPathOrUri)) { - if (repoPathOrUri.repoPath) { - const repo = this._repositories.get(repoPathOrUri.repoPath); - if (repo != null) return repo; - } + if (GitUri.is(repoPathOrUri) && repoPathOrUri.repoPath) { + repo = this.getCachedRepository(repoPathOrUri.repoPath); + if (repo != null) return repo; + } + + filePath = Strings.normalizePath(repoPathOrUri.fsPath); + isVslsScheme = repoPathOrUri.scheme === DocumentSchemes.Vsls; + } + + repo = this.getCachedRepository(filePath); + if (repo != null) return repo; + + let repoPathOrPromise = this._pathToRepoPathCache.get(filePath); + if (repoPathOrPromise !== undefined) { + rp = Promises.is(repoPathOrPromise) ? await repoPathOrPromise : repoPathOrPromise; + // If the repoPath is explicitly null, then we know no repo exists + if (rp === null) return undefined; + + repo = this.getCachedRepository(rp); + // If the repo exists or if we aren't creating it, just return what we found cached + if (!createIfNeeded || repo != null) return repo; + } + + async function findRepoPath(this: GitProviderService): Promise { + const { provider, path } = this.getProvider(filePath); + rp = (await provider.getRepoPath(path)) ?? null; + // Store the found repoPath for this filePath, so we can avoid future lookups for the filePath + this._pathToRepoPathCache.set(filePath, rp); + + if (rp == null) return null; + + // Store the found repoPath for itself, so we can avoid future lookups for the repoPath + this._pathToRepoPathCache.set(rp, rp); - path = repoPathOrUri.fsPath; + if (!createIfNeeded || this._repositories.has(rp)) return rp; + + // If this new repo is inside one of our known roots and we we don't already know about, add it + const root = this.findRepositoryForPath(rp, isVslsScheme); + + let folder; + if (root != null) { + // Not sure why I added this for vsls (I can't see a reason for it anymore), but if it is added it will break submodules + // rp = root.path; + folder = root.folder; } else { - path = repoPathOrUri.fsPath; + folder = workspace.getWorkspaceFolder(GitUri.file(rp, isVslsScheme)); + if (folder == null) { + const parts = rp.split(slash); + folder = { + uri: GitUri.file(rp, isVslsScheme), + name: parts[parts.length - 1], + index: this.repositoryCount, + }; + } } - isVslsScheme = repoPathOrUri.scheme === DocumentSchemes.Vsls; + Logger.log(cc, `Repository found in '${rp}'`); + repo = provider.createRepository(folder, rp, false); + this._repositories.set(rp, repo); + + void this.updateContext(); + // Send a notification that the repositories changed + queueMicrotask(() => this.fireRepositoriesChanged([repo!])); + + return rp; } - const repo = this.findRepositoryForPath(path, isVslsScheme); - if (repo == null) return undefined; + repoPathOrPromise = findRepoPath.call(this); + this._pathToRepoPathCache.set(filePath, repoPathOrPromise); - // Make sure the file is tracked in this repo before returning -- it could be from a submodule - if (!(await this.isTracked(path, repo.path, options))) return undefined; - return repo; + rp = await repoPathOrPromise; + return rp != null ? this.getCachedRepository(rp) : undefined; } @debug() private findRepositoryForPath(path: string, isVslsScheme: boolean | undefined): Repository | undefined { - if (this._repositories.size === 0) return undefined; + if (this.repositoryCount === 0) return undefined; function findBySubPath(repositories: Map, path: string) { const repos = [...repositories.values()].sort((a, b) => a.path.length - b.path.length); @@ -1668,17 +1698,6 @@ export class GitProviderService implements Disposable { return provider.isTrackable(uri); } - private async isTracked( - fileName: string, - repoPath: string | Uri, - options?: { ref?: string; skipCacheUpdate?: boolean }, - ): Promise { - if (options?.ref === GitRevision.deletedOrMissing) return false; - - const { provider, path } = this.getProvider(repoPath); - return provider.isTracked(fileName, path, options); - } - @log() async getDiffTool(repoPath?: string | Uri): Promise { if (repoPath == null) return undefined; diff --git a/src/git/providers/localGitProvider.ts b/src/git/providers/localGitProvider.ts index 929f052..0e9cb0f 100644 --- a/src/git/providers/localGitProvider.ts +++ b/src/git/providers/localGitProvider.ts @@ -23,6 +23,7 @@ import { Container } from '../../container'; import { LogCorrelationContext, Logger } from '../../logger'; import { Messages } from '../../messages'; import { Arrays, debug, Functions, gate, Iterables, log, Paths, Promises, Strings, Versions } from '../../system'; +import { PromiseOrValue } from '../../system/promise'; import { CachedBlame, CachedDiff, @@ -119,7 +120,7 @@ export class LocalGitProvider implements GitProvider, Disposable { private readonly _remotesWithApiProviderCache = new Map | null>(); private readonly _stashesCache = new Map(); private readonly _tagsCache = new Map>(); - private readonly _trackedCache = new Map>(); + private readonly _trackedCache = new Map>(); private readonly _userMapCache = new Map(); constructor(private readonly container: Container) { @@ -258,7 +259,7 @@ export class LocalGitProvider implements GitProvider, Disposable { configuration.getAny( BuiltInGitConfiguration.AutoRepositoryDetection, ) ?? true; - if (autoRepositoryDetection === false) return []; + if (autoRepositoryDetection === false || autoRepositoryDetection === 'openEditors') return []; try { void (await this.ensureGit()); @@ -2079,7 +2080,7 @@ export class LocalGitProvider implements GitProvider, Disposable { key: string, cc: LogCorrelationContext | undefined, ): Promise { - if (!(await this.isTracked(fileName, repoPath, { ref: ref }))) { + if (!(await this.isTracked(fileName, repoPath, ref))) { Logger.log(cc, `Skipping log; '${fileName}' is not tracked`); return emptyPromise as Promise; } @@ -3014,13 +3015,22 @@ export class LocalGitProvider implements GitProvider, Disposable { } } + @gate() @debug() - async getRepoPath(filePath: string, isDirectory: boolean): Promise { + async getRepoPath(filePath: string, isDirectory?: boolean): Promise { const cc = Logger.getCorrelationContext(); let repoPath: string | undefined; try { - const path = isDirectory ? filePath : paths.dirname(filePath); + let path: string; + if (isDirectory) { + path = filePath; + } else { + const stat = await new Promise(resolve => + fs.stat(filePath, (err, stat) => resolve(err == null ? stat : undefined)), + ); + path = stat?.isDirectory() ? filePath : paths.dirname(filePath); + } repoPath = await Git.rev_parse__show_toplevel(path); if (repoPath == null) return repoPath; @@ -3406,69 +3416,57 @@ export class LocalGitProvider implements GitProvider, Disposable { ); } - @log({ exit: tracked => `returned ${tracked}`, singleLine: true }) - async isTracked( - fileNameOrUri: string | GitUri, - repoPath?: string, - options: { ref?: string; skipCacheUpdate?: boolean } = {}, - ): Promise { - if (options.ref === GitRevision.deletedOrMissing) return false; - - let ref = options.ref; + private async isTracked(filePath: string, repoPath?: string, ref?: string): Promise; + private async isTracked(uri: GitUri): Promise; + @log({ exit: tracked => `returned ${tracked}` /*, singleLine: true }*/ }) + private async isTracked(filePathOrUri: string | GitUri, repoPath?: string, ref?: string): Promise { let cacheKey: string; - let fileName: string; - if (typeof fileNameOrUri === 'string') { - [fileName, repoPath] = Paths.splitPath(fileNameOrUri, repoPath); - cacheKey = GitUri.toKey(fileNameOrUri); + let relativeFilePath: string; + + if (typeof filePathOrUri === 'string') { + if (ref === GitRevision.deletedOrMissing) return false; + + cacheKey = ref ? `${ref}:${Strings.normalizePath(filePathOrUri)}` : Strings.normalizePath(filePathOrUri); + [relativeFilePath, repoPath] = Paths.splitPath(filePathOrUri, repoPath); } else { - if (!this.isTrackable(fileNameOrUri)) return false; + if (!this.isTrackable(filePathOrUri)) return false; - fileName = fileNameOrUri.fsPath; - repoPath = fileNameOrUri.repoPath; - ref = fileNameOrUri.sha; - cacheKey = GitUri.toKey(fileName); + // Always use the ref of the GitUri + ref = filePathOrUri.sha; + cacheKey = ref + ? `${ref}:${Strings.normalizePath(filePathOrUri.fsPath)}` + : Strings.normalizePath(filePathOrUri.fsPath); + relativeFilePath = filePathOrUri.fsPath; + repoPath = filePathOrUri.repoPath; } if (ref != null) { - cacheKey += `:${ref}`; + cacheKey = `${ref}:${cacheKey}`; } let tracked = this._trackedCache.get(cacheKey); - if (tracked != null) { - tracked = await tracked; - - return tracked; - } - - tracked = this.isTrackedCore(fileName, repoPath == null ? emptyStr : repoPath, ref); - if (options.skipCacheUpdate) { - tracked = await tracked; - - return tracked; - } + if (tracked != null) return tracked; + tracked = this.isTrackedCore(relativeFilePath, repoPath ?? emptyStr, ref); this._trackedCache.set(cacheKey, tracked); + tracked = await tracked; this._trackedCache.set(cacheKey, tracked); - return tracked; } + @debug() private async isTrackedCore(fileName: string, repoPath: string, ref?: string) { if (ref === GitRevision.deletedOrMissing) return false; try { // Even if we have a ref, check first to see if the file exists (that way the cache will be better reused) - let tracked = Boolean(await Git.ls_files(repoPath == null ? emptyStr : repoPath, fileName)); - if (!tracked && ref != null) { - tracked = Boolean(await Git.ls_files(repoPath == null ? emptyStr : repoPath, fileName, { ref: ref })); + let tracked = Boolean(await Git.ls_files(repoPath, fileName)); + if (!tracked && ref != null && !GitRevision.isUncommitted(ref)) { + tracked = Boolean(await Git.ls_files(repoPath, fileName, { ref: ref })); // If we still haven't found this file, make sure it wasn't deleted in that ref (i.e. check the previous) if (!tracked) { - tracked = Boolean( - await Git.ls_files(repoPath == null ? emptyStr : repoPath, fileName, { - ref: `${ref}^`, - }), - ); + tracked = Boolean(await Git.ls_files(repoPath, fileName, { ref: `${ref}^` })); } } return tracked; diff --git a/src/system/path.ts b/src/system/path.ts index 96955f0..d87b2a0 100644 --- a/src/system/path.ts +++ b/src/system/path.ts @@ -74,19 +74,19 @@ export function isDescendent(uriOrPath: Uri | string, baseUriOrPath: Uri | strin ); } -export function splitPath(fileName: string, repoPath: string | undefined, extract: boolean = true): [string, string] { +export function splitPath(filePath: string, repoPath: string | undefined, extract: boolean = true): [string, string] { if (repoPath) { - fileName = normalizePath(fileName); + filePath = normalizePath(filePath); repoPath = normalizePath(repoPath); const normalizedRepoPath = (repoPath.endsWith(slash) ? repoPath : `${repoPath}/`).toLowerCase(); - if (fileName.toLowerCase().startsWith(normalizedRepoPath)) { - fileName = fileName.substring(normalizedRepoPath.length); + if (filePath.toLowerCase().startsWith(normalizedRepoPath)) { + filePath = filePath.substring(normalizedRepoPath.length); } } else { - repoPath = normalizePath(extract ? paths.dirname(fileName) : repoPath!); - fileName = normalizePath(extract ? paths.basename(fileName) : fileName); + repoPath = normalizePath(extract ? paths.dirname(filePath) : repoPath!); + filePath = normalizePath(extract ? paths.basename(filePath) : filePath); } - return [fileName, repoPath]; + return [filePath, repoPath]; } diff --git a/src/system/promise.ts b/src/system/promise.ts index e0adc7b..62a113f 100644 --- a/src/system/promise.ts +++ b/src/system/promise.ts @@ -2,6 +2,8 @@ import { CancellationToken } from 'vscode'; import { map } from './iterable'; +export type PromiseOrValue = Promise | T; + export class CancellationError = Promise> extends Error { constructor(public readonly promise: T, message: string) { super(message); @@ -81,8 +83,8 @@ export function first(promises: Promise[], predicate: (value: T) => boolea return Promise.race(newPromises); } -export function is(obj: T | Promise): obj is Promise { - return obj != null && typeof (obj as Promise).then === 'function'; +export function is(obj: PromiseLike | T): obj is Promise { + return obj instanceof Promise || typeof (obj as PromiseLike)?.then === 'function'; } export function raceAll(