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

feat(perf): parallelize flag/arg caching #763

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
"cli-progress": "^3.12.0",
"debug": "^4.3.4",
"ejs": "^3.1.8",
"fs-extra": "^9.1.0",
"get-package-type": "^0.1.0",
"globby": "^11.1.0",
"hyperlinker": "^1.0.0",
Expand All @@ -24,7 +23,6 @@
"natural-orderby": "^2.0.3",
"object-treeify": "^1.1.33",
"password-prompt": "^1.1.2",
"semver": "^7.5.3",
"slice-ansi": "^4.0.0",
"string-width": "^4.2.3",
"strip-ansi": "^6.0.1",
Expand All @@ -47,15 +45,13 @@
"@types/chai-as-promised": "^7.1.5",
"@types/clean-stack": "^2.1.1",
"@types/ejs": "^3.1.2",
"@types/fs-extra": "^9.0.13",
"@types/indent-string": "^4.0.1",
"@types/js-yaml": "^3.12.7",
"@types/mocha": "^8.2.3",
"@types/nock": "^11.1.0",
"@types/node": "^16",
"@types/node-notifier": "^8.0.2",
"@types/proxyquire": "^1.3.28",
"@types/semver": "^7.5.0",
"@types/shelljs": "^0.8.11",
"@types/slice-ansi": "^4.0.0",
"@types/strip-ansi": "^5.2.1",
Expand Down Expand Up @@ -117,7 +113,7 @@
"test": "mocha --forbid-only \"test/**/*.test.ts\"",
"test:e2e": "mocha --forbid-only \"test/**/*.e2e.ts\" --timeout 1200000",
"pretest": "yarn build --noEmit && tsc -p test --noEmit --skipLibCheck",
"test:perf": "ts-node test/perf/parser.perf.ts"
"test:perf": "ts-node test/perf/command.perf.ts"
},
"types": "lib/index.d.ts"
}
7 changes: 7 additions & 0 deletions perfToCached.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
yarn run v1.22.19
$ ts-node test/perf/command.perf.ts
toCached (not writing manifest) x 115,355 ops/sec ±3.36% (76 runs sampled)
toCached (writing manifest) x 120,543 ops/sec ±3.19% (79 runs sampled)
slowCommand toCached (not writing manifest) x 9.90 ops/sec ±0.12% (50 runs sampled)
slowCommand (writing manifest) x 9.93 ops/sec ±0.14% (51 runs sampled)
Done in 26.89s.
2 changes: 1 addition & 1 deletion src/cli-ux/action/spinner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as chalk from 'chalk'
import * as supportsColor from 'supports-color'
const stripAnsi = require('strip-ansi')
const ansiStyles = require('ansi-styles')
const ansiEscapes = require('ansi-escapes')
import {errtermwidth} from '../../screen'
import spinners from './spinners'
import {ActionBase, ActionType} from './base'
Expand Down Expand Up @@ -74,6 +73,7 @@ export default class SpinnerAction extends ActionBase {

private _reset() {
if (!this.output) return
const ansiEscapes = require('ansi-escapes')
const lines = this._lines(this.output)
this._write(this.std, ansiEscapes.cursorLeft + ansiEscapes.cursorUp(lines) + ansiEscapes.eraseDown)
this.output = undefined
Expand Down
10 changes: 4 additions & 6 deletions src/cli-ux/config.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import * as semver from 'semver'
import {PJSON} from '../interfaces/pjson'
import {requireJson} from '../util'
import spinner from './action/spinner'
import simple from './action/spinner'
import pride from './action/pride-spinner'
import {ActionBase} from './action/base'

const version = semver.parse(requireJson<PJSON>(__dirname, '..', '..', 'package.json').version)!

export type Levels = 'fatal' | 'error' | 'warn' | 'info' | 'debug' | 'trace'

export interface ConfigMessage {
Expand Down Expand Up @@ -58,9 +55,10 @@ export class Config {
}

function fetch() {
if (globals[version.major]) return globals[version.major]
globals[version.major] = new Config()
return globals[version.major]
const major = requireJson<PJSON>(__dirname, '..', '..', 'package.json').version.split('.')[0]
if (globals[major]) return globals[major]
globals[major] = new Config()
return globals[major]
}

export const config: Config = fetch()
Expand Down
5 changes: 3 additions & 2 deletions src/cli-ux/prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import config from './config'

import * as chalk from 'chalk'
import {stderr} from './stream'
const ansiEscapes = require('ansi-escapes')
const passwordPrompt = require('password-prompt')

export interface IPromptOptions {
prompt?: string;
Expand Down Expand Up @@ -78,6 +76,7 @@ async function single(options: IPromptConfig): Promise<string> {
}

function replacePrompt(prompt: string) {
const ansiEscapes = require('ansi-escapes')
stderr.write(ansiEscapes.cursorHide + ansiEscapes.cursorUp(1) + ansiEscapes.cursorLeft + prompt +
ansiEscapes.cursorDown(1) + ansiEscapes.cursorLeft + ansiEscapes.cursorShow)
}
Expand All @@ -93,6 +92,8 @@ async function _prompt(name: string, inputOptions: Partial<IPromptOptions> = {})
default: '',
...inputOptions,
}
const passwordPrompt = require('password-prompt')

switch (options.type) {
case 'normal':
return normal(options)
Expand Down
126 changes: 53 additions & 73 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import {CompletableOptionFlag, Arg} from '../interfaces/parser'
import {stdout} from '../cli-ux/stream'
import {Performance} from '../performance'
import {settings} from '../settings'
import {userInfo as osUserInfo} from 'node:os'
import {sep} from 'node:path'
import {userInfo as osUserInfo} from 'os'
import {sep} from 'path'

// eslint-disable-next-line new-cap
const debug = Debug()
Expand Down Expand Up @@ -885,71 +885,7 @@ const defaultArgToCached = async (arg: Arg<any>, isWritingManifest = false): Pro
}

export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, isWritingManifest?: boolean): Promise<Command.Cached> {
const flags = {} as {[k: string]: Command.Flag.Cached}

for (const [name, flag] of Object.entries(c.flags || {})) {
if (flag.type === 'boolean') {
flags[name] = {
name,
type: flag.type,
char: flag.char,
summary: flag.summary,
description: flag.description,
hidden: flag.hidden,
required: flag.required,
helpLabel: flag.helpLabel,
helpGroup: flag.helpGroup,
allowNo: flag.allowNo,
dependsOn: flag.dependsOn,
relationships: flag.relationships,
exclusive: flag.exclusive,
deprecated: flag.deprecated,
deprecateAliases: c.deprecateAliases,
aliases: flag.aliases,
delimiter: flag.delimiter,
}
} else {
flags[name] = {
name,
type: flag.type,
char: flag.char,
summary: flag.summary,
description: flag.description,
hidden: flag.hidden,
required: flag.required,
helpLabel: flag.helpLabel,
helpValue: flag.helpValue,
helpGroup: flag.helpGroup,
multiple: flag.multiple,
options: flag.options,
dependsOn: flag.dependsOn,
relationships: flag.relationships,
exclusive: flag.exclusive,
default: await defaultFlagToCached(flag, isWritingManifest),
deprecated: flag.deprecated,
deprecateAliases: c.deprecateAliases,
aliases: flag.aliases,
delimiter: flag.delimiter,
}
// a command-level placeholder in the manifest so that oclif knows it should regenerate the command during help-time
if (typeof flag.defaultHelp === 'function') {
c.hasDynamicHelp = true
}
}
}

const args = {} as {[k: string]: Command.Arg.Cached}
for (const [name, arg] of Object.entries(ensureArgObject(c.args))) {
args[name] = {
name,
description: arg.description,
required: arg.required,
options: arg.options,
default: await defaultArgToCached(arg, isWritingManifest),
hidden: arg.hidden,
}
}

const [flags, args] = await Promise.all([getFlags(c, isWritingManifest), getArgs(c, isWritingManifest)])
const stdProperties = {
id: c.id,
summary: c.summary,
Expand All @@ -971,12 +907,56 @@ export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, i

// do not include these properties in manifest
const ignoreCommandProperties = ['plugin', '_flags', '_enableJsonFlag', '_globalFlags', '_baseFlags']
const stdKeys = Object.keys(stdProperties)
const keysToAdd = Object.keys(c).filter(property => ![...stdKeys, ...ignoreCommandProperties].includes(property))
const additionalProperties: Record<string, unknown> = {}
for (const key of keysToAdd) {
additionalProperties[key] = (c as any)[key]
}
const stdKeysAndIgnored = new Set([...Object.keys(stdProperties), ...ignoreCommandProperties])
const keysToAdd = Object.keys(c).filter(property => !stdKeysAndIgnored.has(property))

const additionalProperties = Object.fromEntries(keysToAdd.map(key => [key, (c as any)[key]]))

return {...stdProperties, ...additionalProperties}
}

const getArgs = async (c: Command.Class, isWritingManifest?: boolean): Promise<Record<string, Command.Arg.Cached>> => {
return Object.fromEntries(await Promise.all(
Object.entries(ensureArgObject(c.args) ?? {})
.map(async ([name, arg]) => ([name, {
name,
description: arg.description,
required: arg.required,
options: arg.options,
default: await defaultArgToCached(arg, isWritingManifest),
hidden: arg.hidden,
}]))))
}

const getFlags = async (c: Command.Class, isWritingManifest?: boolean): Promise<Record<string, Command.Flag.Cached>> => {
return Object.fromEntries(await Promise.all(Object.entries(c.flags || {})
.map(async ([name, flag]) => ([name,
{
name,
type: flag.type,
char: flag.char,
summary: flag.summary,
hidden: flag.hidden,
required: flag.required,
helpLabel: flag.helpLabel,
helpGroup: flag.helpGroup,
description: flag.description,
dependsOn: flag.dependsOn,
relationships: flag.relationships,
exclusive: flag.exclusive,
deprecated: flag.deprecated,
deprecateAliases: c.deprecateAliases,
aliases: flag.aliases,
delimiter: flag.delimiter,
...flag.type === 'boolean' ? {
allowNo: flag.allowNo,
} : {
helpValue: flag.helpValue,
multiple: flag.multiple,
options: flag.options,
default: await defaultFlagToCached(flag, isWritingManifest),
// a command-level placeholder in the manifest so that oclif knows it should regenerate the command during help-time
...typeof flag.defaultHelp === 'function' ? {hasDynamicHelp: true} : {},
},
}]))))
}
8 changes: 2 additions & 6 deletions src/config/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ export class Plugin implements IPlugin {

const manifest = {
version: this.version,
commands: (await Promise.all(this.commandIDs.map(async id => {
commands: Object.fromEntries((await Promise.all(this.commandIDs.map(async id => {
try {
return [id, await toCached(await this.findCommand(id, {must: true}), this, isWritingManifest)]
} catch (error: any) {
Expand All @@ -292,11 +292,7 @@ export class Plugin implements IPlugin {
else throw this.addErrorScope(error, scope)
}
})))
.filter((f): f is [string, Command.Cached] => Boolean(f))
.reduce((commands, [id, c]) => {
commands[id] = c
return commands
}, {} as {[k: string]: Command.Cached}),
.filter((f): f is [string, Command.Cached] => Boolean(f))),
}
marker?.addDetails({fromCache: false, commandCount: Object.keys(manifest.commands).length})
marker?.stop()
Expand Down
6 changes: 3 additions & 3 deletions src/errors/logger.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as fs from 'fs-extra'
import * as path from 'path'
import * as fs from 'fs/promises'
import {dirname} from 'path'
import stripAnsi = require('strip-ansi')

const timestamp = () => new Date().toISOString()
Expand Down Expand Up @@ -34,7 +34,7 @@ export class Logger {
if (this.buffer.length === 0) return
const mylines = this.buffer
this.buffer = []
await fs.mkdirp(path.dirname(this.file))
await fs.mkdir(dirname(this.file), {recursive: true})
await fs.appendFile(this.file, mylines.join('\n') + '\n')
})
await this.flushing
Expand Down
11 changes: 0 additions & 11 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import * as semver from 'semver'

import {Command} from './command'
import {run, execute} from './main'
import {Config, Plugin, tsPath, toCached} from './config'
Expand All @@ -14,7 +12,6 @@ import {Hook} from './interfaces/hooks'
import {settings, Settings} from './settings'
import {HelpSection, HelpSectionRenderer, HelpSectionKeyValueTable} from './help/formatter'
import * as ux from './cli-ux'
import {requireJson} from './util'
import {stderr, stdout} from './cli-ux/stream'
import {Performance} from './performance'

Expand Down Expand Up @@ -62,12 +59,4 @@ function checkCWD() {
}
}

function checkNodeVersion() {
const pjson = requireJson<Interfaces.PJSON>(__dirname, '..', 'package.json')
if (!semver.satisfies(process.versions.node, pjson.engines.node)) {
stderr.write(`WARNING\nWARNING Node version must be ${pjson.engines.node} to use this CLI\nWARNING Current node version: ${process.versions.node}\nWARNING\n`)
}
}

checkCWD()
checkNodeVersion()
8 changes: 4 additions & 4 deletions src/module-loader.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as path from 'path'
import * as url from 'url'
import * as fs from 'fs-extra'
import {existsSync, lstatSync} from 'fs'

import {ModuleLoadError} from './errors'
import {Config as IConfig} from './interfaces'
Expand Down Expand Up @@ -143,10 +143,10 @@ export default class ModuleLoader {

let fileExists = false
let isDirectory = false
if (fs.existsSync(filePath)) {
if (existsSync(filePath)) {
fileExists = true
try {
if (fs.lstatSync(filePath)?.isDirectory?.()) {
if (lstatSync(filePath)?.isDirectory?.()) {
fileExists = false
isDirectory = true
}
Expand Down Expand Up @@ -184,7 +184,7 @@ export default class ModuleLoader {
for (const extension of s_EXTENSIONS) {
const testPath = `${filePath}${extension}`

if (fs.existsSync(testPath)) {
if (existsSync(testPath)) {
return testPath
}
}
Expand Down
8 changes: 4 additions & 4 deletions test/cli-ux/fancy.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {expect, fancy as base, FancyTypes} from 'fancy-test'
import * as fs from 'fs-extra'
import * as path from 'path'
import {rm} from 'fs/promises'
import {join} from 'path'

import {ux} from '../../src'

Expand All @@ -14,8 +14,8 @@ let count = 0
export const fancy = base
.do(async (ctx: {count: number; base: string}) => {
ctx.count = count++
ctx.base = path.join(__dirname, '../tmp', `test-${ctx.count}`)
await fs.remove(ctx.base)
ctx.base = join(__dirname, '../tmp', `test-${ctx.count}`)
await rm(ctx.base, {recursive: true, force: true})
const chalk = require('chalk')
chalk.level = 0
})
Expand Down
4 changes: 2 additions & 2 deletions test/errors/handle.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {expect, fancy} from 'fancy-test'
import * as fs from 'fs-extra'
import {readFileSync} from 'node:fs'
import * as path from 'path'
import * as process from 'process'

Expand Down Expand Up @@ -106,7 +106,7 @@ describe('handle', () => {
handle(new CLIError('uh oh!'))
expect(ctx.stderr).to.equal(` ${x} Error: uh oh!\n`)
await config.errorLogger!.flush()
expect(fs.readFileSync(errlog, 'utf8')).to.contain('Error: uh oh!')
expect(readFileSync(errlog, 'utf8')).to.contain('Error: uh oh!')
expect(process.exitCode).to.equal(2)
})

Expand Down
Loading