Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: optimise template string resolving performance #6685

Merged
merged 43 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
b47e232
perf: optimise template string resolving performance
stefreak Dec 2, 2024
82cc0dd
chore: fix lint issues
vvagaytsev Dec 3, 2024
f4a88a9
test: fix tests
stefreak Dec 3, 2024
c7e0344
chore: fix compilation and lint errors
vvagaytsev Dec 3, 2024
4346ef9
chore: print details on assertion failure in `expectError` function
vvagaytsev Dec 3, 2024
acde6d3
test: fix some assertions
vvagaytsev Dec 3, 2024
8a3e3cb
fix: read raw template string from input
vvagaytsev Dec 3, 2024
08dbd39
test: fix tests for `resolveTemplateString`
vvagaytsev Dec 3, 2024
65a7ae5
chore: return value instead of throwing
vvagaytsev Dec 3, 2024
47b62a5
test: fix remaining resolveTemplateString tests
stefreak Dec 3, 2024
3eb8724
fix: bug fix + unit test
vvagaytsev Dec 3, 2024
56dd850
fix: test failures
vvagaytsev Dec 3, 2024
8aca5ea
fix: always use error message from context if it's available
vvagaytsev Dec 3, 2024
dd1602b
perf: optimize `ConfigContext.resolve`
stefreak Dec 3, 2024
02be20e
perf: don't cache non-template strings and also cache unescape
stefreak Dec 3, 2024
9abba8a
test: fix "exposes arguments and options correctly in command templates"
stefreak Dec 4, 2024
95262d4
fix: throw on critical errors while context resolution
vvagaytsev Dec 4, 2024
8f9daae
test: fix assertions
vvagaytsev Dec 4, 2024
b65c093
test: fix undefined errors in ConfigContext tests
stefreak Dec 4, 2024
dc1205e
test: fix remaining ConfigContext tests
stefreak Dec 4, 2024
1829f26
wip: use static analysis instead of scancontext
stefreak Dec 4, 2024
6fa3fea
chore: lint fixes
vvagaytsev Dec 4, 2024
2cf06d3
chore: replace `ScanContext` with `NoOpContext`
vvagaytsev Dec 4, 2024
d25d761
test: consume generator in tests
vvagaytsev Dec 4, 2024
e122a37
test: fix "throws if action kind is not resolvable"
stefreak Dec 4, 2024
29152fa
feat: allow referencing variables and evaluating complex expressions …
stefreak Dec 4, 2024
fc231be
refactor: get rid of function `collectTemplateReferences`
vvagaytsev Dec 4, 2024
498dc30
refactor: introduce local variables
vvagaytsev Dec 5, 2024
2ba9932
fix: module dependencies resolition
vvagaytsev Dec 5, 2024
4b4b787
fix: restore module name filter
vvagaytsev Dec 5, 2024
b40f5e6
test: update assertion for circular dep error
stefreak Dec 5, 2024
1f7d99c
chore: remove dead code
vvagaytsev Dec 5, 2024
03006d1
fix: handle all kinds of `GardenError` on the AST level
vvagaytsev Dec 5, 2024
4e51588
test: fix partial runtime resolution tests
stefreak Dec 5, 2024
f96b534
fix: make sure that yaml path is included in template string errors
stefreak Dec 5, 2024
77f4e5b
refactor: make code safer and easier to reason about
stefreak Dec 5, 2024
930b04a
improvement: minor improvements
stefreak Dec 5, 2024
3397b0e
test: additional tests
stefreak Dec 5, 2024
e220046
refactor: remove NoopContext
stefreak Dec 5, 2024
3a71187
fix: handle further edge cases in `getContextLookupReferences`
stefreak Dec 6, 2024
f9f9e43
fix: handle optional expressions in brackets correctly
stefreak Dec 6, 2024
2caced0
fix: special edge case in module resolution flow
stefreak Dec 6, 2024
d25be9f
chore: fix lint
stefreak Dec 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions core/src/config/render-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,29 +273,29 @@ async function renderConfigs({
context: RenderTemplateConfigContext
renderConfig: RenderTemplateConfig
}): Promise<TemplatableConfig[]> {
const source = { yamlDoc: template.internal.yamlDoc, path: ["configs"] }
vvagaytsev marked this conversation as resolved.
Show resolved Hide resolved

const templateDescription = `${configTemplateKind} '${template.name}'`
const templateConfigs = template.configs || []
const partiallyResolvedTemplateConfigs = resolveTemplateStrings({
value: templateConfigs,
context,
contextOpts: { allowPartial: true },
source: { yamlDoc: template.internal.yamlDoc, path: ["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 {
const result = resolveTemplateString({ string: m.name, context, contextOpts: { allowPartial: false } })
if (typeof result === "string") {
resolvedName = result
} else {
throw new ConfigurationError({
message: "must resolve to string",
})
}
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
53 changes: 34 additions & 19 deletions core/src/graph/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import cloneDeep from "fast-copy"
import { isEqual, mapValues, memoize, omit, pick, uniq } from "lodash-es"
import { isEqual, isString, mapValues, memoize, omit, pick, uniq } from "lodash-es"
import type {
Action,
ActionConfig,
Expand Down Expand Up @@ -68,6 +68,7 @@ import { profileAsync } from "../util/profiling.js"
import { uuidv4 } from "../util/random.js"
import { getSourcePath } from "../vcs/vcs.js"
import { styles } from "../logger/styles.js"
import { isUnresolvableValue } from "../template-string/static-analysis.js"

function* sliceToBatches<T>(dict: Record<string, T>, batchSize: number) {
const entries = Object.entries(dict)
Expand Down Expand Up @@ -1034,35 +1035,49 @@ function dependenciesFromActionConfig({
for (const ref of getActionTemplateReferences(config, templateContext)) {
let needsExecuted = false

const outputKey = ref.fullRef[4] as string
const outputType = ref.keyPath[0]

if (isUnresolvableValue(outputType)) {
const err = outputType.getError()
throw new ConfigurationError({
message: `Found invalid action reference: ${err}`,
})
}

const outputKey = ref.keyPath[1]

// also avoid execution when referencing the static output keys of the ref action type.
// e.g. a helm deploy referencing container build static output deploymentImageName
// ${actions.build.my-container.outputs.deploymentImageName}
const refActionKey = actionReferenceToString(ref)
const refActionType = configsByKey[refActionKey]?.type
let refStaticOutputKeys: string[] = []
if (refActionType) {
const refActionSpec = actionTypes[ref.kind][refActionType]?.spec
refStaticOutputKeys = refActionSpec?.staticOutputsSchema
? describeSchema(refActionSpec.staticOutputsSchema).keys
: []
}

if (
ref.fullRef[3] === "outputs" &&
!staticOutputKeys.includes(outputKey) &&
!refStaticOutputKeys.includes(outputKey)
) {
needsExecuted = true
if (outputType === "outputs") {
let refStaticOutputKeys: string[] = []
if (refActionType) {
const refActionSpec = actionTypes[ref.kind][refActionType]?.spec
refStaticOutputKeys = refActionSpec?.staticOutputsSchema
? describeSchema(refActionSpec.staticOutputsSchema).keys
: []
}

// Avoid execution when referencing the static output keys of the ref action type.
// e.g. a helm deploy referencing container build static output deploymentImageName
// ${actions.build.my-container.outputs.deploymentImageName}
if (!isString(outputKey)) {
needsExecuted = true
} else {
needsExecuted = !staticOutputKeys.includes(outputKey) && !refStaticOutputKeys.includes(outputKey)
}
}

const refWithType = {
...ref,
type: refActionType,
}

addDep(refWithType, { explicit: false, needsExecutedOutputs: needsExecuted, needsStaticOutputs: !needsExecuted })
addDep(omit(refWithType, ["keyPath"]), {
explicit: false,
needsExecutedOutputs: needsExecuted,
needsStaticOutputs: !needsExecuted,
})
}

return deps
Expand Down
5 changes: 1 addition & 4 deletions core/src/outputs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,9 @@ export async function resolveProjectOutputs(garden: Garden, log: Log): Promise<O
})
)
for (const finding of generator) {
if (finding.type === "unresolvable") {
continue
}
const keyPath = finding.keyPath
const refName = keyPath[1]
if (!refName) {
if (!refName || !isString(refName)) {
continue
}

Expand Down
20 changes: 15 additions & 5 deletions core/src/template-string/static-analysis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ export function* visitAll({
}
}

export class UnresolvableValue {
constructor(public readonly getError: () => GardenError) {}
}

export function isUnresolvableValue(
val: CollectionOrValue<TemplatePrimitive | UnresolvableValue>
): val is UnresolvableValue {
return val instanceof UnresolvableValue
}

export type ContextLookupReferenceFinding =
| {
type: "resolvable"
Expand All @@ -90,7 +100,7 @@ export type ContextLookupReferenceFinding =
}
| {
type: "unresolvable"
keyPath: (string | number | { getError: () => GardenError })[]
keyPath: (string | number | UnresolvableValue)[]
yamlSource: ConfigSource
}

Expand Down Expand Up @@ -126,17 +136,17 @@ export function* getContextLookupReferences(
})
if (typeof key === "symbol") {
isResolvable = false
return {
getError: captureError(() =>
return new UnresolvableValue(
captureError(() =>
// this will throw an error, because the key could not be resolved
keyPathExpression.evaluate({
context,
opts: { allowPartial: false },
optional: false,
yamlSource,
})
),
}
)
)
}
return key
})
Expand Down
101 changes: 54 additions & 47 deletions core/src/template-string/template-string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
NoOpContext,
} from "../config/template-contexts/base.js"
import cloneDeep from "fast-copy"
import { difference, isNumber, isPlainObject, isString } from "lodash-es"
import { difference, isPlainObject, isString } from "lodash-es"
import type { ActionReference, Primitive, StringMap } from "../config/common.js"
import {
arrayConcatKey,
Expand Down Expand Up @@ -44,8 +44,8 @@ import type { ObjectPath } from "../config/base.js"
import type { TemplatePrimitive } from "./types.js"
import * as ast from "./ast.js"
import { LRUCache } from "lru-cache"
import type { ContextLookupReferenceFinding } from "./static-analysis.js"
import { getContextLookupReferences, visitAll } from "./static-analysis.js"
import type { ContextLookupReferenceFinding, UnresolvableValue } from "./static-analysis.js"
import { getContextLookupReferences, isUnresolvableValue, visitAll } from "./static-analysis.js"
import type { ModuleConfig } from "../config/module.js"

const escapePrefix = "$${"
Expand Down Expand Up @@ -609,113 +609,120 @@ export function mayContainTemplateString(obj: any): boolean {
}

interface ActionTemplateReference extends ActionReference {
fullRef: ContextKeySegment[]
keyPath: (ContextKeySegment | UnresolvableValue)[]
}

export function extractActionReference(finding: ContextLookupReferenceFinding): ActionTemplateReference {
if (finding.type === "unresolvable") {
for (const k of finding.keyPath) {
if (typeof k === "object" && "getError" in k) {
const err = k.getError()
throw new ConfigurationError({
message: `Found invalid action reference: ${err.message}`,
})
}
}
throw new InternalError({
message: "No error found in unresolvable finding",
const kind = finding.keyPath[1]
if (!kind) {
throw new ConfigurationError({
message: `Found invalid action reference (missing kind).`,
})
}

if (!finding.keyPath[1]) {
if (isUnresolvableValue(kind)) {
const err = kind.getError()
throw new ConfigurationError({
message: `Found invalid action reference (missing kind).`,
message: `Found invalid action reference: ${err.message}`,
})
}

if (!isString(finding.keyPath[1])) {
if (!isString(kind)) {
throw new ConfigurationError({
message: `Found invalid action reference (kind is not a string).`,
})
}

if (!actionKindsLower.includes(finding.keyPath[1])) {
if (!actionKindsLower.includes(kind)) {
throw new ConfigurationError({
message: `Found invalid action reference (invalid kind '${finding.keyPath[1]}')`,
message: `Found invalid action reference (invalid kind '${kind}')`,
})
}

if (!finding.keyPath[2]) {
const name = finding.keyPath[2]
if (!name) {
throw new ConfigurationError({
message: "Found invalid action reference (missing name)",
})
}

if (!isString(finding.keyPath[2])) {
if (isUnresolvableValue(name)) {
const err = name.getError()
throw new ConfigurationError({
message: `Found invalid action reference: ${err.message}`,
})
}

if (!isString(name)) {
throw new ConfigurationError({
message: "Found invalid action reference (name is not a string)",
})
}

return {
kind: <ActionKind>titleize(finding.keyPath[1]),
name: finding.keyPath[2],
fullRef: finding.keyPath,
kind: <ActionKind>titleize(kind),
name,
keyPath: finding.keyPath.slice(3),
}
}

export function extractRuntimeReference(finding: ContextLookupReferenceFinding): ActionTemplateReference {
if (finding.type === "unresolvable") {
for (const k of finding.keyPath) {
if (typeof k === "object" && "getError" in k) {
const err = k.getError()
throw new ConfigurationError({
message: `Found invalid action reference: ${err.message}`,
})
}
}
throw new InternalError({
message: "No error found in unresolvable finding",
const runtimeKind = finding.keyPath[1]
if (!runtimeKind) {
throw new ConfigurationError({
message: "Found invalid runtime reference (missing kind)",
})
}

if (!finding.keyPath[1]) {
if (isUnresolvableValue(runtimeKind)) {
const err = runtimeKind.getError()
throw new ConfigurationError({
message: "Found invalid runtime reference (missing kind)",
message: `Found invalid runtime reference: ${err.message}`,
})
}
if (!isString(finding.keyPath[1])) {

if (!isString(runtimeKind)) {
throw new ConfigurationError({
message: "Found invalid runtime reference (kind is not a string)",
})
}

let kind: ActionKind
if (finding.keyPath[1] === "services") {
if (runtimeKind === "services") {
kind = "Deploy"
} else if (finding.keyPath[1] === "tasks") {
} else if (runtimeKind === "tasks") {
kind = "Run"
} else {
throw new ConfigurationError({
message: `Found invalid runtime reference (invalid kind '${finding.keyPath[1]}')`,
message: `Found invalid runtime reference (invalid kind '${runtimeKind}')`,
})
}

if (!finding.keyPath[2]) {
const name = finding.keyPath[2]

if (!name) {
throw new ConfigurationError({
message: `Found invalid runtime reference (missing name)`,
})
}
if (!isString(finding.keyPath[2])) {

if (isUnresolvableValue(name)) {
const err = name.getError()
throw new ConfigurationError({
message: `Found invalid action reference: ${err.message}`,
})
}

if (!isString(name)) {
throw new ConfigurationError({
message: "Found invalid runtime reference (name is not a string)",
})
}

return {
kind,
name: finding.keyPath[2],
fullRef: finding.keyPath,
name,
keyPath: finding.keyPath.slice(3),
}
}

Expand Down Expand Up @@ -773,7 +780,7 @@ export function getModuleTemplateReferences(config: ModuleConfig, context: Modul
}

const moduleName = keyPath[1]
if (!isString(moduleName) && !isNumber(moduleName)) {
if (isUnresolvableValue(moduleName)) {
const err = moduleName.getError()
throw new ConfigurationError({
message: `Found invalid module reference: ${err.message}`,
Expand Down
12 changes: 11 additions & 1 deletion core/src/util/testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,17 @@ export function expectError(fn: Function, assertion: ExpectErrorAssertion = {})
expectFuzzyMatch(
errorMessage,
contains,
`Assertion failed: '${errorMessage}' does not contain '${contains}'.\n\nOriginal error: ${toGardenError(err).stack}`
dedent`
Expected

'${stripAnsi(errorMessage).toLowerCase()}'

to contain

'${typeof contains === "string" ? stripAnsi(contains).toLowerCase() : contains.map(stripAnsi).map((s) => s.toLowerCase())}'

Original error:
${stripAnsi(toGardenError(err).stack || "<no stack>")}`
)
}

Expand Down
Loading