Gitrest: Add RedisFs HashMap Optimization (microsoft#19220)

## Description Adds a new version of `RedisFs` that stores the entire document repo in 1 Redis HashMap using HGet/HSet. This will drastically improve performance when deleting an entire document, because we can delete the root key instead of each individual key. ### Bonus Extends the Unit tests added and improved in microsoft#18221 to incorporate RedisFs _and_ HashMapRedisFs.
dannimad · Jan 16, 2024 · 6a54c4d · 6a54c4d
1 parent bc356b8
commit 6a54c4d
Show file tree

Hide file tree

Showing 19 changed files with 438 additions and 164 deletions.
diff --git a/server/charts/historian/templates/gitrest-configmap.yaml b/server/charts/historian/templates/gitrest-configmap.yaml
@@ -65,6 +65,7 @@ data:
             "enableOptimizedInitialSummary": {{ .Values.gitrest.git.enableOptimizedInitialSummary }},
             "enableSlimGitInit": {{ .Values.gitrest.git.enableSlimGitInit }},
             "enableRedisFsMetrics": {{ .Values.gitrest.git.enableRedisFsMetrics }},
+            "enableHashmapRedisFs": {{ .Values.gitrest.git.enableHashmapRedisFs }},
             "redisApiMetricsSamplingPeriod": {{ .Values.gitrest.git.redisApiMetricsSamplingPeriod }}
         },
         "redis": {

diff --git a/server/charts/historian/values.yaml b/server/charts/historian/values.yaml
@@ -69,6 +69,7 @@ gitrest:
     enableOptimizedInitialSummary: false
     enableSlimGitInit: false
     enableRedisFsMetrics: false
+    enableHashmapRedisFs: false
     redisApiMetricsSamplingPeriod: 0
   redis:
     host: redis

diff --git a/server/gitrest/packages/gitrest-base/package.json b/server/gitrest/packages/gitrest-base/package.json
@@ -86,6 +86,7 @@
 		"@types/debug": "^4.1.5",
 		"@types/express": "^4.11.0",
 		"@types/express-serve-static-core": "^4.17.32",
+		"@types/ioredis-mock": "^8.2.5",
 		"@types/lorem-ipsum": "^1.0.2",
 		"@types/mocha": "^10.0.0",
 		"@types/nconf": "^0.10.0",
@@ -99,6 +100,7 @@
 		"c8": "^8.0.1",
 		"concurrently": "^8.2.1",
 		"eslint": "~8.27.0",
+		"ioredis-mock": "^8.9.0",
 		"lorem-ipsum": "^1.0.6",
 		"mocha": "^10.1.0",
 		"rimraf": "^3.0.2",

diff --git a/server/gitrest/packages/gitrest-base/src/routes/git/blobs.ts b/server/gitrest/packages/gitrest-base/src/routes/git/blobs.ts
@@ -37,9 +37,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -66,9 +67,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/git/commits.ts b/server/gitrest/packages/gitrest-base/src/routes/git/commits.ts
@@ -40,9 +40,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -66,9 +67,10 @@ export function create(
 		const resultP = repoManagerFactory
 			.open(repoManagerParams)
 			.then(async (repoManager) => {
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/git/refs.ts b/server/gitrest/packages/gitrest-base/src/routes/git/refs.ts
@@ -48,9 +48,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -73,9 +74,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -105,9 +107,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -134,9 +137,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -163,9 +167,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/git/tags.ts b/server/gitrest/packages/gitrest-base/src/routes/git/tags.ts
@@ -40,9 +40,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -65,9 +66,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/git/trees.ts b/server/gitrest/packages/gitrest-base/src/routes/git/trees.ts
@@ -38,9 +38,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,
@@ -63,9 +64,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/repository/commits.ts b/server/gitrest/packages/gitrest-base/src/routes/repository/commits.ts
@@ -41,9 +41,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/repository/contents.ts b/server/gitrest/packages/gitrest-base/src/routes/repository/contents.ts
@@ -33,9 +33,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				await checkSoftDeleted(
 					fsManager,
 					repoManager.path,

diff --git a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts
@@ -304,9 +304,10 @@ export function create(
 						fileSystemManagerFactories,
 						repoManagerParams.isEphemeralContainer,
 					);
-					const fsManager = fileSystemManagerFactory.create(
-						repoManagerParams.fileSystemManagerParams,
-					);
+					const fsManager = fileSystemManagerFactory.create({
+						...repoManagerParams.fileSystemManagerParams,
+						rootDir: repoManager.path,
+					});
 					await checkSoftDeleted(
 						fsManager,
 						repoManager.path,
@@ -379,9 +380,10 @@ export function create(
 					fileSystemManagerFactories,
 					repoManagerParams.isEphemeralContainer,
 				);
-				const fsManager = fileSystemManagerFactory.create(
-					repoManagerParams.fileSystemManagerParams,
-				);
+				const fsManager = fileSystemManagerFactory.create({
+					...repoManagerParams.fileSystemManagerParams,
+					rootDir: repoManager.path,
+				});
 				// A new document cannot already be soft-deleted.
 				if (!optimizeForInitialSummary) {
 					await checkSoftDeleted(
@@ -434,9 +436,10 @@ export function create(
 						fileSystemManagerFactories,
 						repoManagerParams.isEphemeralContainer,
 					);
-					const fsManager = fileSystemManagerFactory.create(
-						repoManagerParams.fileSystemManagerParams,
-					);
+					const fsManager = fileSystemManagerFactory.create({
+						...repoManagerParams.fileSystemManagerParams,
+						rootDir: repoManager.path,
+					});
 					return deleteSummary(
 						repoManager,
 						fsManager,