Skip to content

Commit

Permalink
perf: improve graph resolve performance (#6670)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Vladimir Vagaytsev <[email protected]>
  • Loading branch information
@stefreak @vvagaytsev
3 people authored Nov 25, 2024
1 parent 2a7b69a commit 4e4cd91
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 51 deletions.
4 changes: 4 additions & 0 deletions core/src/commands/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -158,6 +159,7 @@ type DataCallback = (data: string) => void
export type CommandArgsType<C extends Command> = C extends Command<infer Args, any> ? Args : never
export type CommandOptionsType<C extends Command> = C extends Command<any, infer Opts> ? Opts : never
export type CommandResultType<C extends Command> = C extends Command<any, any, infer R> ? R : never

export abstract class Command<
A extends ParameterObject = ParameterObject,
O extends ParameterObject = ParameterObject,
Expand Down Expand Up @@ -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({
Expand Down
125 changes: 77 additions & 48 deletions core/src/config/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -560,6 +561,16 @@ const _loadYaml = profile(function _loadYaml(data: Buffer) {
return load(data.toString()) as PrimitiveMap
})

const loadVarfileCache = new LRUCache<string, Promise<PrimitiveMap>>({
max: 10000,
ttl: 30000,
ttlAutopurge: true,
})

export function clearVarfileCache() {
loadVarfileCache.clear()
}

export const loadVarfile = profileAsync(async function loadVarfile({
configRoot,
path,
Expand All @@ -573,63 +584,81 @@ export const loadVarfile = profileAsync(async function loadVarfile({
defaultPath: string | undefined
optional?: boolean
log?: Log
}): Promise<PrimitiveMap> {
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, <string>(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<PrimitiveMap> | undefined = loadVarfileCache.get(resolvedPath)
if (!promise) {
promise = loadVarfileInner()
loadVarfileCache.set(resolvedPath, promise)
}

return await promise

async function loadVarfileInner(): Promise<PrimitiveMap> {
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}`,
})
}
}
})
8 changes: 5 additions & 3 deletions core/src/template-string/template-string.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,8 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings<T
} else if (Array.isArray(value)) {
const output: unknown[] = []

value.forEach((v, i) => {
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(
Expand Down Expand Up @@ -337,7 +338,7 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings<T
} else {
output.push(resolveTemplateStrings({ value: v, context, contextOpts, source, path: path && [...path, i] }))
}
})
}

return <T>(<unknown>output)
} else if (isPlainObject(value)) {
Expand All @@ -351,7 +352,8 @@ export const resolveTemplateStrings = profile(function resolveTemplateStrings<T
// Resolve $merge keys, depth-first, leaves-first
let output = {}

for (const [k, v] of Object.entries(value)) {
for (const k in value as Record<string, unknown>) {
const v = value[k]
const resolved = resolveTemplateStrings({ value: v, context, contextOpts, source, path: path && [...path, k] })

if (k === objectSpreadKey) {
Expand Down
6 changes: 6 additions & 0 deletions core/test/unit/src/actions/action-configs-to-graph.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down

0 comments on commit 4e4cd91

Please sign in to comment.