From 4e4cd91e01ccd0dba3a38a045c41762f62587613 Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Mon, 25 Nov 2024 15:26:10 +0100 Subject: [PATCH] perf: improve graph resolve performance (#6670) * perf: make sure we do not concurrently read the same varfile multiple times Also cache the promise in LRUCache for up to 30 seconds. Co-authored-by: Vladimir Vagaytsev * perf: use plain loops instead of `Object.entries` and `Array.forEach` in `resolveTemplateStrings` * chore(test): fix test data init and cleanup * chore: clear varfiles cache before each command execution in dev console --------- Co-authored-by: Vladimir Vagaytsev Co-authored-by: Vladimir Vagaytsev <10628074+vvagaytsev@users.noreply.github.com> --- core/src/commands/base.ts | 4 + core/src/config/base.ts | 125 +++++++++++------- core/src/template-string/template-string.ts | 8 +- .../src/actions/action-configs-to-graph.ts | 6 + 4 files changed, 92 insertions(+), 51 deletions(-) diff --git a/core/src/commands/base.ts b/core/src/commands/base.ts index acf17d3fda..2241a524e5 100644 --- a/core/src/commands/base.ts +++ b/core/src/commands/base.ts @@ -41,6 +41,7 @@ import type { AnalyticsHandler } from "../analytics/analytics.js" import { withSessionContext } from "../util/open-telemetry/context.js" import { wrapActiveSpan } from "../util/open-telemetry/spans.js" import { styles } from "../logger/styles.js" +import { clearVarfileCache } from "../config/base.js" export interface CommandConstructor { new (parent?: CommandGroup): Command @@ -158,6 +159,7 @@ type DataCallback = (data: string) => void export type CommandArgsType = C extends Command ? Args : never export type CommandOptionsType = C extends Command ? Opts : never export type CommandResultType = C extends Command ? R : never + export abstract class Command< A extends ParameterObject = ParameterObject, O extends ParameterObject = ParameterObject, @@ -375,6 +377,8 @@ export abstract class Command< // Clear the VCS handler's tree cache to make sure we pick up any changed sources. // FIXME: use file watching to be more surgical here, this is suboptimal garden.treeCache.invalidateDown(log, ["path"]) + // also clear the cached varfiles + clearVarfileCache() log.silly(() => `Starting command '${this.getFullName()}' action`) result = await this.action({ diff --git a/core/src/config/base.ts b/core/src/config/base.ts index 965e005dc0..6850bd9979 100644 --- a/core/src/config/base.ts +++ b/core/src/config/base.ts @@ -34,6 +34,7 @@ import { dedent, deline } from "../util/string.js" import { makeDocsLinkStyled } from "../docs/common.js" import { profile, profileAsync } from "../util/profiling.js" import { readFile } from "fs/promises" +import { LRUCache } from "lru-cache" export const configTemplateKind = "ConfigTemplate" export const renderTemplateKind = "RenderTemplate" @@ -560,6 +561,16 @@ const _loadYaml = profile(function _loadYaml(data: Buffer) { return load(data.toString()) as PrimitiveMap }) +const loadVarfileCache = new LRUCache>({ + max: 10000, + ttl: 30000, + ttlAutopurge: true, +}) + +export function clearVarfileCache() { + loadVarfileCache.clear() +} + export const loadVarfile = profileAsync(async function loadVarfile({ configRoot, path, @@ -573,63 +584,81 @@ export const loadVarfile = profileAsync(async function loadVarfile({ defaultPath: string | undefined optional?: boolean log?: Log -}): Promise { - if (!path && !defaultPath) { +}) { + const pathOrDefault = path || defaultPath + if (!pathOrDefault) { throw new ParameterError({ message: `Neither a path nor a defaultPath was provided. Config root: ${configRoot}`, }) } - const resolvedPath = resolve(configRoot, (path || defaultPath)) + const resolvedPath = resolve(configRoot, pathOrDefault) - try { - const data = await _readFile(resolvedPath) - log?.silly(() => `Loaded ${data.length} bytes from varfile ${resolvedPath}`) - const relPath = relative(configRoot, resolvedPath) - const filename = basename(resolvedPath.toLowerCase()) - - if (filename.endsWith(".json")) { - // JSON parser throws a JSON syntax error on completely empty input file, - // and returns an empty object for an empty JSON. - const parsed = JSON.parse(data.toString()) - if (!isPlainObject(parsed)) { - throw new ConfigurationError({ - message: `Configured variable file ${relPath} must be a valid plain JSON object. Got: ${typeof parsed}`, - }) + let promise: Promise | undefined = loadVarfileCache.get(resolvedPath) + if (!promise) { + promise = loadVarfileInner() + loadVarfileCache.set(resolvedPath, promise) + } + + return await promise + + async function loadVarfileInner(): Promise { + try { + const data = await _readFile(resolvedPath) + log?.silly(() => `Loaded ${data.length} bytes from varfile ${resolvedPath}`) + const relPath = relative(configRoot, resolvedPath) + const filename = basename(resolvedPath.toLowerCase()) + + if (filename.endsWith(".json")) { + // JSON parser throws a JSON syntax error on completely empty input file, + // and returns an empty object for an empty JSON. + const parsed = JSON.parse(data.toString()) + if (!isPlainObject(parsed)) { + throw new ConfigurationError({ + message: `Configured variable file ${relPath} must be a valid plain JSON object. Got: ${typeof parsed}`, + }) + } + return parsed as PrimitiveMap + } else if (filename.endsWith(".yml") || filename.endsWith(".yaml")) { + // YAML parser returns `undefined` for empty files, we interpret that as an empty object. + const parsed = _loadYaml(data) || {} + if (!isPlainObject(parsed)) { + throw new ConfigurationError({ + message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof parsed}`, + }) + } + return parsed as PrimitiveMap + } else { + // Note: For backwards-compatibility we fall back on using .env as a default format, + // and don't specifically validate the extension for that. + // The dotenv parser returns an empty object for invalid or empty input file. + const parsed = dotenv.parse(data) + return parsed as PrimitiveMap } - return parsed as PrimitiveMap - } else if (filename.endsWith(".yml") || filename.endsWith(".yaml")) { - // YAML parser returns `undefined` for empty files, we interpret that as an empty object. - const parsed = _loadYaml(data) || {} - if (!isPlainObject(parsed)) { - throw new ConfigurationError({ - message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof parsed}`, - }) + } catch (error) { + if (error instanceof ConfigurationError) { + throw error } - return parsed as PrimitiveMap - } else { - // Note: For backwards-compatibility we fall back on using .env as a default format, - // and don't specifically validate the extension for that. - // The dotenv parser returns an empty object for invalid or empty input file. - const parsed = dotenv.parse(data) - return parsed as PrimitiveMap - } - } catch (error) { - if (error instanceof ConfigurationError) { - throw error - } - if (isErrnoException(error) && error.code === "ENOENT") { - if (path && path !== defaultPath && !optional) { - throw new ConfigurationError({ - message: `Could not find varfile at path '${path}'. Absolute path: ${resolvedPath}`, - }) - } else { - return {} + if (isErrnoException(error) && error.code === "ENOENT") { + if ( + // if path is defined, we are loading explicitly configured varfile. + path && + // if the user explicitly declares default path (e.g. garden.env) then we do not throw. + path !== defaultPath && + !optional + ) { + throw new ConfigurationError({ + message: `Could not find varfile at path '${path}'. Absolute path: ${resolvedPath}`, + }) + } else { + // The default var file did not exist. In that case we return empty object. + return {} + } } - } - throw new ConfigurationError({ - message: `Unable to load varfile at '${path}': ${error}`, - }) + throw new ConfigurationError({ + message: `Unable to load varfile at '${path}': ${error}`, + }) + } } }) diff --git a/core/src/template-string/template-string.ts b/core/src/template-string/template-string.ts index bee20bad97..c23053cba1 100644 --- a/core/src/template-string/template-string.ts +++ b/core/src/template-string/template-string.ts @@ -295,7 +295,8 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings { + for (let i = 0; i < value.length; i++) { + const v = value[i] if (isPlainObject(v) && v[arrayConcatKey] !== undefined) { if (Object.keys(v).length > 1) { const extraKeys = naturalList( @@ -337,7 +338,7 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings(output) } else if (isPlainObject(value)) { @@ -351,7 +352,8 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings) { + const v = value[k] const resolved = resolveTemplateStrings({ value: v, context, contextOpts, source, path: path && [...path, k] }) if (k === objectSpreadKey) { diff --git a/core/test/unit/src/actions/action-configs-to-graph.ts b/core/test/unit/src/actions/action-configs-to-graph.ts index 9c74f03587..d0953cc41c 100644 --- a/core/test/unit/src/actions/action-configs-to-graph.ts +++ b/core/test/unit/src/actions/action-configs-to-graph.ts @@ -21,6 +21,7 @@ import { DEFAULT_TEST_TIMEOUT_SEC, } from "../../../../src/constants.js" import { getRemoteSourceLocalPath } from "../../../../src/util/ext-source-util.js" +import { clearVarfileCache } from "../../../../src/config/base.js" describe("actionConfigsToGraph", () => { let tmpDir: TempDirectory @@ -34,6 +35,11 @@ describe("actionConfigsToGraph", () => { log = garden.log }) + afterEach(() => { + // Some tests re-use and re-write existing varfiles, so we need to clear the cache explicitly. + clearVarfileCache() + }) + it("resolves a Build action", async () => { const graph = await actionConfigsToGraph({ garden,