Skip to content

Commit

Permalink
fix(core): performance issue with certain dependency structures
Browse files Browse the repository at this point in the history
This change reduces the number of times module configs are resolved,
which especially helps for deeply nested dependency graphs. Should
dramatically reduce CPU usage in certain cases (although most users
won't feel much of a difference).
  • Loading branch information
edvald authored and thsig committed Aug 20, 2021
1 parent b36dddf commit b72de2e
Show file tree
Hide file tree
Showing 37 changed files with 262 additions and 97 deletions.
26 changes: 17 additions & 9 deletions core/src/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -717,12 +717,13 @@ export class ActionRouter implements TypeGuard {

async getServiceStatuses({
log,
graph,
serviceNames,
}: {
log: LogEntry
graph: ConfigGraph
serviceNames?: string[]
}): Promise<ServiceStatusMap> {
const graph = await this.garden.getConfigGraph(log)
const services = graph.getServices({ names: serviceNames })

const tasks = services.map(
Expand Down Expand Up @@ -942,7 +943,7 @@ export class ActionRouter implements TypeGuard {
actionType: T
defaultHandler?: ModuleActionHandlers[T]
}): Promise<ModuleActionOutputs[T]> {
const { module, pluginName, log } = params
const { module, pluginName, log, graph } = params

log.silly(`Getting '${actionType}' handler for module '${module.name}' (type '${module.type}')`)

Expand All @@ -954,7 +955,6 @@ export class ActionRouter implements TypeGuard {
})

const providers = await this.garden.resolveProviders(log)
const graph = await this.garden.getConfigGraph(log)
const templateContext = ModuleConfigContext.fromModule({
garden: this.garden,
resolvedProviders: providers,
Expand Down Expand Up @@ -983,7 +983,7 @@ export class ActionRouter implements TypeGuard {
actionType: T
defaultHandler?: ServiceActionHandlers[T]
}) {
let { log, service, runtimeContext } = params
let { log, service, runtimeContext, graph } = params
let module = service.module

log.silly(`Getting ${actionType} handler for service ${service.name}`)
Expand All @@ -996,7 +996,6 @@ export class ActionRouter implements TypeGuard {
})

const providers = await this.garden.resolveProviders(log)
const graph = await this.garden.getConfigGraph(log, runtimeContext)

const modules = graph.getModules()
const templateContext = ModuleConfigContext.fromModule({
Expand All @@ -1016,6 +1015,9 @@ export class ActionRouter implements TypeGuard {
if (!runtimeContextIsEmpty && getRuntimeTemplateReferences(module).length > 0) {
log.silly(`Resolving runtime template strings for service '${service.name}'`)

// Resolve the graph again (TODO: avoid this somehow!)
graph = await this.garden.getConfigGraph(log, runtimeContext)

// Resolve the service again
service = graph.getService(service.name)
module = service.module
Expand Down Expand Up @@ -1049,7 +1051,7 @@ export class ActionRouter implements TypeGuard {
actionType: T
defaultHandler?: TaskActionHandlers[T]
}) {
let { task, log } = params
let { task, log, graph } = params
const runtimeContext = params["runtimeContext"] as RuntimeContext | undefined
let module = task.module

Expand All @@ -1063,7 +1065,6 @@ export class ActionRouter implements TypeGuard {
})

const providers = await this.garden.resolveProviders(log)
const graph = await this.garden.getConfigGraph(log, runtimeContext)

const modules = graph.getModules()
const templateContext = ModuleConfigContext.fromModule({
Expand All @@ -1083,6 +1084,9 @@ export class ActionRouter implements TypeGuard {
if (!runtimeContextIsEmpty && getRuntimeTemplateReferences(module).length > 0) {
log.silly(`Resolving runtime template strings for task '${task.name}'`)

// Resolve the graph again (TODO: avoid this somehow!)
graph = await this.garden.getConfigGraph(log, runtimeContext)

// Resolve the task again
task = graph.getTask(task.name)
module = task.module
Expand Down Expand Up @@ -1441,14 +1445,18 @@ type WrappedModuleActionMap = {
// avoid having to specify common params on each action helper call
type ActionRouterParams<T extends PluginActionParamsBase> = Omit<T, CommonParams> & { pluginName?: string }

type ModuleActionRouterParams<T extends PluginModuleActionParamsBase> = Omit<T, CommonParams> & { pluginName?: string }
// additionally make runtimeContext param optional
type ModuleActionRouterParams<T extends PluginModuleActionParamsBase> = Omit<T, CommonParams> & {
graph: ConfigGraph
pluginName?: string
}

type ServiceActionRouterParams<T extends PluginServiceActionParamsBase> = Omit<T, "module" | CommonParams> & {
graph: ConfigGraph
pluginName?: string
}

type TaskActionRouterParams<T extends PluginTaskActionParamsBase> = Omit<T, "module" | CommonParams> & {
graph: ConfigGraph
pluginName?: string
}

Expand Down
9 changes: 8 additions & 1 deletion core/src/commands/call.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,14 @@ export class CallCommand extends Command<Args> {
// No need for full context, since we're just checking if the service is running.
const runtimeContext = emptyRuntimeContext
const actions = await garden.getActionRouter()
const status = await actions.getServiceStatus({ service, log, devMode: false, hotReload: false, runtimeContext })
const status = await actions.getServiceStatus({
service,
log,
graph,
devMode: false,
hotReload: false,
runtimeContext,
})

if (!includes(["ready", "outdated"], status.state)) {
throw new RuntimeError(`Service ${service.name} is not running`, {
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export class ExecCommand extends Command<Args> {
const actions = await garden.getActionRouter()
const result = await actions.execInService({
log,
graph,
service,
command,
interactive: opts.interactive,
Expand Down
7 changes: 4 additions & 3 deletions core/src/commands/get/get-status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,10 @@ export class GetStatusCommand extends Command {

async action({ garden, log, opts }: CommandParams): Promise<CommandResult<StatusCommandResult>> {
const actions = await garden.getActionRouter()
const graph = await garden.getConfigGraph(log)

const envStatus = await garden.getEnvironmentStatus(log)
const serviceStatuses = await actions.getServiceStatuses({ log })
const serviceStatuses = await actions.getServiceStatuses({ log, graph })

let result: StatusCommandResult = {
providers: envStatus,
Expand All @@ -80,7 +81,6 @@ export class GetStatusCommand extends Command {
}

if (opts.output) {
const graph = await garden.getConfigGraph(log)
result = {
...result,
...(await Bluebird.props({
Expand Down Expand Up @@ -125,6 +125,7 @@ async function getTestStatuses(garden: Garden, configGraph: ConfigGraph, log: Lo
const result = await actions.getTestResult({
module,
log,
graph: configGraph,
test: testFromConfig(module, testConfig, configGraph),
})
return [`${module.name}.${testConfig.name}`, runStatus(result)]
Expand All @@ -140,7 +141,7 @@ async function getTaskStatuses(garden: Garden, configGraph: ConfigGraph, log: Lo

return fromPairs(
await Bluebird.map(tasks, async (task) => {
const result = await actions.getTaskResult({ task, log })
const result = await actions.getTaskResult({ task, log, graph: configGraph })
return [task.name, runStatus(result)]
})
)
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/get/get-task-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export class GetTaskResultCommand extends Command<Args> {
const taskResult = await actions.getTaskResult({
log,
task,
graph,
})

let result: GetTaskResultCommandResult = null
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/get/get-test-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export class GetTestResultCommand extends Command<Args> {

const testResult = await actions.getTestResult({
log,
graph,
test,
module,
})
Expand Down
2 changes: 1 addition & 1 deletion core/src/commands/logs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export class LogsCommand extends Command<Args, Opts> {
const actions = await garden.getActionRouter()

await Bluebird.map(services, async (service: GardenService<any>) => {
await actions.getServiceLogs({ log, service, stream, follow, tail, since })
await actions.getServiceLogs({ log, graph, service, stream, follow, tail, since })
})

const sorted = sortBy(result, "timestamp")
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/run/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export class RunModuleCommand extends Command<Args, Opts> {

const result = await actions.runModule({
log,
graph,
module,
command: opts.command?.split(" "),
args: args.arguments || [],
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/run/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export class RunServiceCommand extends Command<Args, Opts> {

const result = await actions.runService({
log,
graph,
service,
runtimeContext,
interactive,
Expand Down
1 change: 1 addition & 0 deletions core/src/commands/run/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ export class RunTestCommand extends Command<Args, Opts> {

const result = await actions.testModule({
log,
graph,
module,
silent: false,
interactive,
Expand Down
2 changes: 1 addition & 1 deletion core/src/config-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export type DependencyGraph = { [key: string]: DependencyGraphNode }
* A graph data structure that facilitates querying (recursive or non-recursive) of the project's dependency and
* dependant relationships.
*
* This should be initialized with fully resolved and validated ModuleConfigs.
* This should be initialized with resolved and validated GardenModules.
*/
export class ConfigGraph {
private dependencyGraph: DependencyGraph
Expand Down
2 changes: 1 addition & 1 deletion core/src/plugins/kubernetes/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ export class PodRunner extends PodRunnerParams {
})
}

await sleep(200)
await sleep(800)
}

// Retrieve logs after run
Expand Down
2 changes: 2 additions & 0 deletions core/src/plugins/kubernetes/system.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,11 @@ interface GetSystemServicesStatusParams {

export async function getSystemServiceStatus({ sysGarden, log, serviceNames }: GetSystemServicesStatusParams) {
const actions = await sysGarden.getActionRouter()
const graph = await sysGarden.getConfigGraph(log)

const serviceStatuses = await actions.getServiceStatuses({
log: log.placeholder({ level: LogLevel.verbose, childEntriesInheritLevel: true }),
graph,
serviceNames,
})
const state = combineStates(Object.values(serviceStatuses).map((s) => (s && s.state) || "unknown"))
Expand Down
41 changes: 29 additions & 12 deletions core/src/proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { registerCleanupFunction, sleep } from "./util/util"
import { LogEntry } from "./logger/log-entry"
import { GetPortForwardResult } from "./types/plugin/service/getPortForward"
import { LocalAddress } from "./db/entities/local-address"
import { ConfigGraph } from "./config-graph"

interface PortProxy {
key: string
Expand All @@ -42,39 +43,55 @@ registerCleanupFunction("kill-service-port-proxies", () => {

const portLock = new AsyncLock()

export async function startPortProxies(garden: Garden, log: LogEntry, service: GardenService, status: ServiceStatus) {
// tslint:disable-next-line: max-line-length
export async function startPortProxies({
garden,
graph,
log,
service,
status,
}: {
garden: Garden
graph: ConfigGraph
log: LogEntry
service: GardenService
status: ServiceStatus
}) {
if (garden.disablePortForwards) {
log.info({ msg: chalk.gray("Port forwards disabled") })
return []
}

return Bluebird.map(status.forwardablePorts || [], (spec) => {
return startPortProxy(garden, log, service, spec)
return startPortProxy({ garden, graph, log, service, spec })
})
}

async function startPortProxy(garden: Garden, log: LogEntry, service: GardenService, spec: ForwardablePort) {
interface StartPortProxyParams {
garden: Garden
graph: ConfigGraph
log: LogEntry
service: GardenService
spec: ForwardablePort
}

async function startPortProxy({ garden, graph, log, service, spec }: StartPortProxyParams) {
const key = getPortKey(service, spec)
let proxy = activeProxies[key]

if (!proxy) {
// Start new proxy
proxy = activeProxies[key] = await createProxy(garden, log, service, spec)
proxy = activeProxies[key] = await createProxy({ garden, graph, log, service, spec })
} else if (!isEqual(proxy.spec, spec)) {
// Stop existing proxy and create new one
stopPortProxy(proxy, log)
proxy = activeProxies[key] = await createProxy(garden, log, service, spec)
proxy = activeProxies[key] = await createProxy({ garden, graph, log, service, spec })
}

return proxy
}

async function createProxy(
garden: Garden,
log: LogEntry,
service: GardenService,
spec: ForwardablePort
): Promise<PortProxy> {
async function createProxy({ garden, graph, log, service, spec }: StartPortProxyParams): Promise<PortProxy> {
const actions = await garden.getActionRouter()
const key = getPortKey(service, spec)
let fwd: GetPortForwardResult | null = null
Expand All @@ -91,7 +108,7 @@ async function createProxy(
log.debug(`Starting port forward to ${key}`)

try {
fwd = await actions.getPortForward({ service, log, ...spec })
fwd = await actions.getPortForward({ service, log, graph, ...spec })
} catch (err) {
log.error(`Error starting port forward to ${key}: ${err.message}`)
}
Expand Down
2 changes: 1 addition & 1 deletion core/src/task-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export class TaskGraph extends EventEmitter2 {
throw new TaskGraphError(
dedent`
${failed.length} task(s) failed:
${failed.map(([key, result]) => `- ${key}: ${result?.error?.toString()}`).join("\n")}
${failed.map(([key, result]) => `- ${key}: ${result?.error?.stack || result?.error?.message}`).join("\n")}
`,
{ results }
)
Expand Down
3 changes: 2 additions & 1 deletion core/src/tasks/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export class BuildTask extends BaseTask {
status: "active",
})

const status = await actions.getBuildStatus({ log: this.log, module })
const status = await actions.getBuildStatus({ log: this.log, graph: this.graph, module })

if (status.ready) {
log.setSuccess({
Expand All @@ -148,6 +148,7 @@ export class BuildTask extends BaseTask {
let result: BuildResult
try {
result = await actions.build({
graph: this.graph,
module,
log,
})
Expand Down
2 changes: 1 addition & 1 deletion core/src/tasks/delete-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class DeleteServiceTask extends BaseTask {
let status: ServiceStatus

try {
status = await actions.deleteService({ log: this.log, service: this.service })
status = await actions.deleteService({ log: this.log, service: this.service, graph: this.graph })
} catch (err) {
this.log.setError()
throw err
Expand Down
10 changes: 9 additions & 1 deletion core/src/tasks/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ export class DeployTask extends BaseTask {
const taskResultTasks = await Bluebird.map(deps.run, async (task) => {
return new GetTaskResultTask({
garden: this.garden,
graph: this.graph,
log: this.log,
task,
force: false,
Expand Down Expand Up @@ -207,6 +208,7 @@ export class DeployTask extends BaseTask {
} else {
try {
status = await actions.deployService({
graph: this.graph,
service: this.service,
runtimeContext,
log,
Expand All @@ -230,7 +232,13 @@ export class DeployTask extends BaseTask {
}

if (this.garden.persistent) {
const proxies = await startPortProxies(this.garden, log, this.service, status)
const proxies = await startPortProxies({
garden: this.garden,
graph: this.graph,
log,
service: this.service,
status,
})

for (const proxy of proxies) {
const targetHost = proxy.spec.targetName || this.service.name
Expand Down
Loading

0 comments on commit b72de2e

Please sign in to comment.