Skip to content

Commit

Permalink
perf: optimise template string resolving performance (#6685)
Browse files Browse the repository at this point in the history
* perf: optimise template string resolving performance

We optimise the template string resolving performance by
making sure that we only parse each template expression once. Once
parsed, we use the AST to evaluate template expressions.

* chore: fix lint issues

* test: fix tests

* chore: fix compilation and lint errors

* chore: print details on assertion failure in `expectError` function

* test: fix some assertions

* fix: read raw template string from input

* test: fix tests for `resolveTemplateString`

* chore: return value instead of throwing

* test: fix remaining resolveTemplateString tests

Co-authored-by: Vladimir Vagaytsev <[email protected]>

* fix: bug fix + unit test

* fix: test failures

* fix: always use error message from context if it's available

* perf: optimize `ConfigContext.resolve`

* perf: don't cache non-template strings and also cache unescape

* test: fix "exposes arguments and options correctly in command templates"

* fix: throw on critical errors while context resolution

* test: fix assertions

* test: fix undefined errors in ConfigContext tests

Co-authored-by: Vladimir Vagaytsev <[email protected]>

* test: fix remaining ConfigContext tests

* wip: use static analysis instead of scancontext

ScanContext does not work with AST, because it does not eagerly resolve
all variables anymore. we have to walk the AST intead.

* chore: lint fixes

* chore: replace `ScanContext` with `NoOpContext`

* test: consume generator in tests

* test: fix "throws if action kind is not resolvable"

* feat: allow referencing variables and evaluating complex expressions in action references

* refactor: get rid of function `collectTemplateReferences`

Co-authored-by: Steffen Neubauer <[email protected]>

* refactor: introduce local variables

* fix: module dependencies resolition

* fix: restore module name filter

* test: update assertion for circular dep error

* chore: remove dead code

* fix: handle all kinds of `GardenError` on the AST level

* test: fix partial runtime resolution tests

* fix: make sure that yaml path is included in template string errors

Co-authored-by: Vladimir Vagaytsev <[email protected]>

* refactor: make code safer and easier to reason about

* improvement: minor improvements

* test: additional tests

* refactor: remove NoopContext

* fix: handle further edge cases in `getContextLookupReferences`

* fix: handle optional expressions in brackets correctly

* fix: special edge case in module resolution flow

* chore: fix lint

---------

Co-authored-by: Vladimir Vagaytsev <[email protected]>
Co-authored-by: Vladimir Vagaytsev <[email protected]>
  • Loading branch information
3 people authored Dec 6, 2024
1 parent ce902db commit a348564
Show file tree
Hide file tree
Showing 34 changed files with 2,738 additions and 1,419 deletions.
2 changes: 1 addition & 1 deletion core/.mocharc.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require:
- build/test/setup.js
- ../../../../build/test/setup.js
watch-files:
- build/**/*
ignore:
Expand Down
16 changes: 8 additions & 8 deletions core/src/commands/custom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class CustomCommandWrapper extends Command {
const commandVariables = resolveTemplateStrings({
value: this.spec.variables,
context: variablesContext,
source: { yamlDoc, basePath: ["variables"] },
source: { yamlDoc, path: ["variables"] },
})
const variables: any = jsonMerge(cloneDeep(garden.variables), commandVariables)

Expand All @@ -133,18 +133,18 @@ export class CustomCommandWrapper extends Command {
if (this.spec.exec) {
const startedAt = new Date()

const exec = validateWithPath({
const exec = validateWithPath<CommandResource["exec"]>({
config: resolveTemplateStrings({
value: this.spec.exec,
context: commandContext,
source: { yamlDoc, basePath: ["exec"] },
source: { yamlDoc, path: ["exec"] },
}),
schema: customCommandExecSchema(),
path: this.spec.internal.basePath,
projectRoot: garden.projectRoot,
configType: `exec field in custom Command '${this.name}'`,
source: undefined,
})
})!

const command = exec.command
log.debug(`Running exec command: ${command.join(" ")}`)
Expand Down Expand Up @@ -185,18 +185,18 @@ export class CustomCommandWrapper extends Command {
if (this.spec.gardenCommand) {
const startedAt = new Date()

let gardenCommand = validateWithPath({
let gardenCommand = validateWithPath<CommandResource["gardenCommand"]>({
config: resolveTemplateStrings({
value: this.spec.gardenCommand,
context: commandContext,
source: { yamlDoc, basePath: ["gardenCommand"] },
source: { yamlDoc, path: ["gardenCommand"] },
}),
schema: customCommandGardenCommandSchema(),
path: this.spec.internal.basePath,
projectRoot: garden.projectRoot,
configType: `gardenCommand field in custom Command '${this.name}'`,
source: undefined,
})
})!

log.debug(`Running Garden command: ${gardenCommand.join(" ")}`)

Expand Down Expand Up @@ -291,7 +291,7 @@ export async function getCustomCommands(log: Log, projectRoot: string) {
path: (<CommandResource>config).internal.basePath,
projectRoot,
configType: `custom Command '${config.name}'`,
source: { yamlDoc: (<CommandResource>config).internal.yamlDoc },
source: { yamlDoc: (<CommandResource>config).internal.yamlDoc, path: [] },
})
)

Expand Down
2 changes: 1 addition & 1 deletion core/src/commands/get/get-modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ function filterSecrets<T extends object>(object: T, secrets: StringMap): T {
const secretValues = new Set(Object.values(secrets))
const secretNames = Object.keys(secrets)
const sanitized = <T>deepMap(object, (value) => {
if (secretValues.has(value)) {
if (typeof value === "string" && secretValues.has(value)) {
const name = secretNames.find((n) => secrets[n] === value)!
return `[filtered secret: ${name}]`
} else {
Expand Down
6 changes: 3 additions & 3 deletions core/src/commands/workflow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class WorkflowCommand extends Command<Args, {}> {
const files = resolveTemplateStrings({
value: workflow.files || [],
context: templateContext,
source: { yamlDoc, basePath: ["files"] },
source: { yamlDoc, path: ["files"] },
})

// Write all the configured files for the workflow
Expand Down Expand Up @@ -175,12 +175,12 @@ export class WorkflowCommand extends Command<Args, {}> {
step.command = resolveTemplateStrings({
value: step.command,
context: stepTemplateContext,
source: { yamlDoc, basePath: ["steps", index, "command"] },
source: { yamlDoc, path: ["steps", index, "command"] },
}).filter((arg) => !!arg)

stepResult = await runStepCommand(stepParams)
} else if (step.script) {
step.script = resolveTemplateString({ string: step.script, context: stepTemplateContext })
step.script = resolveTemplateString({ string: step.script, context: stepTemplateContext }) as string
stepResult = await runStepScript(stepParams)
} else {
stepResult = undefined
Expand Down
6 changes: 3 additions & 3 deletions core/src/config/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import type { Document, DocumentOptions } from "yaml"
import { parseAllDocuments } from "yaml"
import { dedent, deline } from "../util/string.js"
import { makeDocsLinkStyled } from "../docs/common.js"
import { profile, profileAsync } from "../util/profiling.js"
import { profileAsync } from "../util/profiling.js"
import { readFile } from "fs/promises"
import { LRUCache } from "lru-cache"

Expand Down Expand Up @@ -557,9 +557,9 @@ const _readFile = profileAsync(async function _readFile(path: string) {
return await readFile(path)
})

const _loadYaml = profile(function _loadYaml(data: Buffer) {
function _loadYaml(data: Buffer) {
return load(data.toString()) as PrimitiveMap
})
}

const loadVarfileCache = new LRUCache<string, Promise<PrimitiveMap>>({
max: 10000,
Expand Down
6 changes: 5 additions & 1 deletion core/src/config/config-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ export async function resolveConfigTemplate(
const loggedIn = garden.isLoggedIn()
const enterpriseDomain = garden.cloudApi?.domain
const context = new ProjectConfigContext({ ...garden, loggedIn, enterpriseDomain })
const resolved = resolveTemplateStrings({ value: partial, context, source: { yamlDoc: resource.internal.yamlDoc } })
const resolved = resolveTemplateStrings({
value: partial,
context,
source: { yamlDoc: resource.internal.yamlDoc, path: [] },
})
const configPath = resource.internal.configFilePath

// Validate the partial config
Expand Down
6 changes: 3 additions & 3 deletions core/src/config/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ export function resolveProjectConfig({
secrets,
commandInfo,
}),
source: { yamlDoc: config.internal.yamlDoc, basePath: [] },
source: { yamlDoc: config.internal.yamlDoc, path: [] },
})
} catch (err) {
log.error("Failed to resolve project configuration.")
Expand Down Expand Up @@ -622,7 +622,7 @@ export const pickEnvironment = profileAsync(async function _pickEnvironment({
})
const projectVariables: DeepPrimitiveMap = <any>merge(projectConfig.variables, projectVarfileVars)

const source = { yamlDoc: projectConfig.internal.yamlDoc, basePath: ["environments", index] }
const source = { yamlDoc: projectConfig.internal.yamlDoc, path: ["environments", index] }

// Resolve template strings in the environment config, except providers
environmentConfig = resolveTemplateStrings({
Expand All @@ -642,7 +642,7 @@ export const pickEnvironment = profileAsync(async function _pickEnvironment({
source,
})

environmentConfig = validateWithPath({
environmentConfig = validateWithPath<EnvironmentConfig>({
config: environmentConfig,
schema: environmentSchema(),
configType: `environment ${environment}`,
Expand Down
65 changes: 45 additions & 20 deletions core/src/config/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,18 @@

import { deline } from "../util/string.js"
import {
joiIdentifier,
joiUserIdentifier,
joiArray,
createSchema,
joi,
joiArray,
joiIdentifier,
joiIdentifierMap,
joiSparseArray,
createSchema,
joiUserIdentifier,
} from "./common.js"
import { collectTemplateReferences } from "../template-string/template-string.js"
import { ConfigurationError } from "../exceptions.js"
import type { ModuleConfig } from "./module.js"
import { moduleConfigSchema } from "./module.js"
import { memoize, uniq } from "lodash-es"
import { isNumber, isString, memoize, uniq } from "lodash-es"
import type { GardenPluginSpec } from "../plugin/plugin.js"
import type { EnvironmentStatus } from "../plugin/handlers/Provider/getEnvironmentStatus.js"
import { environmentStatusSchema } from "./status.js"
Expand All @@ -30,6 +29,8 @@ import type { ActionState } from "../actions/types.js"
import type { ValidResultType } from "../tasks/base.js"
import { uuidv4 } from "../util/random.js"
import { s } from "./zod.js"
import { getContextLookupReferences, visitAll } from "../template-string/static-analysis.js"
import type { ConfigContext } from "./template-contexts/base.js"

// TODO: dedupe from the joi schema below
export const baseProviderConfigSchemaZod = s.object({
Expand Down Expand Up @@ -164,34 +165,58 @@ export function providerFromConfig({
* Given a plugin and its provider config, return a list of dependency names based on declared dependencies,
* as well as implicit dependencies based on template strings.
*/
export function getAllProviderDependencyNames(plugin: GardenPluginSpec, config: GenericProviderConfig) {
export function getAllProviderDependencyNames(
plugin: GardenPluginSpec,
config: GenericProviderConfig,
context: ConfigContext
) {
return uniq([
...(plugin.dependencies || []).map((d) => d.name),
...(config.dependencies || []),
...getProviderTemplateReferences(config),
...getProviderTemplateReferences(config, context),
]).sort()
}

/**
* Given a provider config, return implicit dependencies based on template strings.
*/
export function getProviderTemplateReferences(config: GenericProviderConfig) {
const references = collectTemplateReferences(config)
export function getProviderTemplateReferences(config: GenericProviderConfig, context: ConfigContext) {
const deps: string[] = []

for (const key of references) {
if (key[0] === "providers") {
const providerName = key[1] as string
if (!providerName) {
throw new ConfigurationError({
message: deline`
Invalid template key '${key.join(".")}' in configuration for provider '${config.name}'. You must
const generator = getContextLookupReferences(
visitAll({
value: config,
parseTemplateStrings: true,
// TODO: get proper source
source: {
path: [],
},
}),
context
)
for (const finding of generator) {
const keyPath = finding.keyPath
if (keyPath[0] !== "providers") {
continue
}

const providerName = keyPath[1]
if (!providerName || isNumber(providerName)) {
throw new ConfigurationError({
message: deline`s
Invalid template key '${keyPath.join(".")}' in configuration for provider '${config.name}'. You must
specify a provider name as well (e.g. \${providers.my-provider}).
`,
})
}
deps.push(providerName)
})
}
if (!isString(providerName)) {
const err = providerName.getError()
throw new ConfigurationError({
message: `Found invalid provider reference: ${err.message}`,
})
}

deps.push(providerName)
}

return uniq(deps).sort()
Expand Down
19 changes: 13 additions & 6 deletions core/src/config/render-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,15 @@ export async function renderConfigTemplate({
const resolvedWithoutInputs = resolveTemplateStrings({
value: { ...omit(config, "inputs") },
context: templateContext,
source: { yamlDoc },
source: { yamlDoc, path: [] },
})
const partiallyResolvedInputs = resolveTemplateStrings({
value: config.inputs || {},
context: templateContext,
contextOpts: {
allowPartial: true,
},
source: { yamlDoc, basePath: ["inputs"] },
source: { yamlDoc, path: ["inputs"] },
})
let resolved: RenderTemplateConfig = {
...resolvedWithoutInputs,
Expand Down Expand Up @@ -214,7 +214,7 @@ async function renderModules({
value: m,
context,
contextOpts: { allowPartial: true },
source: { yamlDoc, basePath: ["modules", i] },
source: { yamlDoc, path: ["modules", i] },
})
const renderConfigPath = renderConfig.internal.configFilePath || renderConfig.internal.basePath

Expand Down Expand Up @@ -273,22 +273,29 @@ async function renderConfigs({
context: RenderTemplateConfigContext
renderConfig: RenderTemplateConfig
}): Promise<TemplatableConfig[]> {
const source = { yamlDoc: template.internal.yamlDoc, path: ["configs"] }

const templateDescription = `${configTemplateKind} '${template.name}'`
const templateConfigs = template.configs || []
const partiallyResolvedTemplateConfigs = resolveTemplateStrings({
value: templateConfigs,
context,
contextOpts: { allowPartial: true },
source: { yamlDoc: template.internal.yamlDoc, basePath: ["inputs"] },
source,
})

return Promise.all(
partiallyResolvedTemplateConfigs.map(async (m) => {
partiallyResolvedTemplateConfigs.map(async (m, index) => {
// Resolve just the name, which must be immediately resolvable
let resolvedName = m.name

try {
resolvedName = resolveTemplateString({ string: m.name, context, contextOpts: { allowPartial: false } })
resolvedName = resolveTemplateString({
string: m.name,
context,
contextOpts: { allowPartial: false },
source: { ...source, path: [...source.path, index, "name"] },
}) as string
} catch (error) {
throw new ConfigurationError({
message: `Could not resolve the \`name\` field (${m.name}) for a config in ${templateDescription}: ${error}\n\nNote that template strings in config names in must be fully resolvable at the time of scanning. This means that e.g. references to other actions, modules or runtime outputs cannot be used.`,
Expand Down
Loading

0 comments on commit a348564

Please sign in to comment.