Sm/perf-gains #726
Merged
Sm/perf-gains #726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mshanemc
commented
Jul 10, 2023
|
|
||
| const failed = results.filter(r => r.status === 'failed') | ||
| if (failed.length > 0) throw new FailedFlagValidationError({parse, failed}) | ||
| } | ||
|
|
||
| async function resolveFlags(flags: FlagRelationship[]): Promise<Record<string, unknown>> { | ||
| if (cachedResolvedFlags) return cachedResolvedFlags |
There was a problem hiding this comment.
this could be called a lot of times, so I memoized it
mshanemc
commented
Jul 10, 2023
| const flags = relationship.flags ?? [] | ||
| const results = [] | ||
| function validateRelationships(name: string, flag: CompletableFlag<any>): Promise<Validation>[] { | ||
| return ((flag.relationships ?? []).map(relationship => { |
There was a problem hiding this comment.
returns an array of promises. No need to await them--someone else can do that later.
|
This issue has been linked to a new work item: W-13734277 |
| } else { | ||
| flags[token.flag] = true | ||
| try { | ||
| if (flag.type === 'boolean') { |
There was a problem hiding this comment.
why do this check if it's a boolean, looks like it's the same call on L223 as 226
There was a problem hiding this comment.
pulled it down and looked at it... that's weird though
There was a problem hiding this comment.
yeah--the oclif types are a bit confused.
| @@ -81,15 +87,11 @@ export async function validate(parse: { | |||
| return result ? [flag.name, parse.output.flags[flag.name]] : null | |||
| }) | |||
| const resolved = await Promise.all(promises) | |||
| return Object.fromEntries(resolved.filter(r => r !== null) as [string, unknown][]) | |||
| cachedResolvedFlags = Object.fromEntries(resolved.filter(r => r !== null) as [string, unknown][]) | |||
There was a problem hiding this comment.
return Object.fromEntries...?
There was a problem hiding this comment.
this could be called a lot of times, so I memoized it
|
it is faster 🏎️ 🏁 ➜ ts-node test/perf/parser.perf.ts
simple x 267,234 ops/sec ±5.49% (85 runs sampled)
multiple async flags that take time x 9.86 ops/sec ±0.23% (50 runs sampled)
flagstravaganza x 14,873 ops/sec ±2.47% (86 runs sampled)
➜ core git:(sm/perf-gains)
➜ ts-node test/perf/parser.perf.ts
simple x 266,510 ops/sec ±5.54% (85 runs sampled)
multiple async flags that take time x 9.85 ops/sec ±0.11% (49 runs sampled)
flagstravaganza x 14,844 ops/sec ±2.37% (85 runs sampled)
➜ core git:(sm/perf-gains)
➜ gco main src
Updated 3 paths from 569eb75
➜ core git:(sm/perf-gains) ✗
➜ yb
yarn run v1.22.19
$ shx rm -rf lib && tsc
✨ Done in 2.42s.
➜ core git:(sm/perf-gains) ✗
➜ ts-node test/perf/parser.perf.ts
simple x 216,811 ops/sec ±5.08% (85 runs sampled)
multiple async flags that take time x 4.92 ops/sec ±0.11% (28 runs sampled)
flagstravaganza x 12,968 ops/sec ±2.84% (87 runs sampled)
➜ core git:(sm/perf-gains) ✗
➜ ts-node test/perf/parser.perf.ts
simple x 213,781 ops/sec ±5.69% (84 runs sampled)
multiple async flags that take time x 4.92 ops/sec ±0.12% (28 runs sampled)
flagstravaganza x 12,806 ops/sec ±2.86% (86 runs sampled) |
WillieRuemmele
approved these changes
Jul 11, 2023
QA Notes✅ : validated with NUTs ➜ ../../oss/plugin-release-management/bin/dev github:check:closed
Error (1): The following error occurred:
Missing required flag github-token✅ : read flag value ➜ ../../oss/plugin-release-management/bin/dev github:check:closed --github-token abc
Error (1): Bad credentials✅ : required flag validated ➜ ../../oss/plugin-release-management/bin/dev github:check:closed
Error (1): The following error occurred:
Missing required flag github-token✅ : read from env ➜ GITHUB_TOKEN=abc ../../oss/plugin-release-management/bin/dev github:check:closed
Error (1): Bad credentials✅ : relationship flags still validated ➜ ../../oss/plugin-deploy-retrieve/bin/dev project:deploy:start -d force-app --results-dir abc
Error (1): The following error occurred:
One of the following must be provided when using --results-dir: --coverage-formatters, --junit |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rewrite of Parse and Validate (flags only, not args)
this branch:
$ ts-node test/perf/parser.perf.ts
simple x 131,268 ops/sec ±1.15% (79 runs sampled)
multiple async flags that take time x 9.90 ops/sec ±0.14% (50 runs sampled)
flagstravaganza x 7,425 ops/sec ±2.10% (83 runs sampled)
main
$ ts-node test/perf/parser.perf.ts
simple x 103,392 ops/sec ±0.91% (77 runs sampled)
multiple async flags that take time x 4.96 ops/sec ±0.13% (28 runs sampled)
multiple async flags that take time x 5,451 ops/sec ±2.09% (79 runs sampled)
lots of refactoring was needed to enable parallelization, but the main goal is to run flags' async parse methods in parallel.
The code could be a lot cleaner if we could make the assumption that this.context on Parser is not mutating.
That middle perf test
multiple async flags that take timeis meant to model something that's got some delay on it (ex, various local fs operations like sf's Org flag)