-
Notifications
You must be signed in to change notification settings - Fork 11.9k
feat(benchmark): add benchmark package and travis job #4133
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
Conversation
8daba22
to
a7d4e5f
Compare
This command should instead be a separate package. |
c1bfc51
to
cf8d7a7
Compare
cf8d7a7
to
6716db0
Compare
} | ||
|
||
// Returns an array of `length` length, filled with `undefined` | ||
export function makeArray(length: number) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.from()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
} | ||
|
||
// Returns average of all numbers in `arr` | ||
export function average(arr: any[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have lodash, _.mean()
would work.
import { BenchmarkOptions } from './benchmark-options'; | ||
|
||
// Log to console and logFile if it exists | ||
export function benchmarkLog(benchmarkOptions: BenchmarkOptions, str: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional for this; could you use the @ngtools/logger
library?
@@ -0,0 +1,15 @@ | |||
{ | |||
"name": "@ngtools/benchmark", | |||
"version": "0.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you set the version to 0.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// Returns a promise with the result of calling `times` times the `fn` function with `args` | ||
// `fn` will be called in parallel, and is expected to return a promise | ||
// Not used currently | ||
export function parallelMultiPromise(times: number, fn: any, ...args: string[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it initially but there was no command that could benefit from being ran in parallel. I'll remove it.
} from './utils'; | ||
|
||
export const benchmark = (benchmarkOptions: BenchmarkOptions) => { | ||
const log = benchmarkLog.bind(null, benchmarkOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's use @ngtools/logger
.
serialMultiPromise | ||
} from './utils'; | ||
|
||
export const benchmark = (benchmarkOptions: BenchmarkOptions) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
flagCombinations.unshift([]); | ||
|
||
const startTime = Date.now(); | ||
log(`Base command: ${benchmarkOptions.command}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use a JSON.stringify
, or maybe a for...in
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order wouldn't be guaranteed with those, nor would they allow to show a more verbose text as easily.
childProcess.on('exit', (err: number) => { | ||
result.time = Date.now() - startTime; | ||
return err && !result.overrideErr | ||
? resolve({ err, stdout, stderr }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, still I since want to gather the remaining results. If you're doing a lot of iterations you don't want to throw everything away just because one failed.
default: [], | ||
alias: 'ea' | ||
}, | ||
'match': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like these would always be the same for commands, right? Like for the serve
command. I'm not sure, just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It varies a bit... on some tests I wanted to benchmark the difference in rebuilds between the main chunk and lazy loaded chunks. Or to check the rebuild time for css/html. In those cases the written string also changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added css/html/script tests
In case you're missing it or never saw it: https://lodash.com/docs/ |
@hansl can you elaborate on which parts could do with being replaced with lodash? Seems excessive to add it just to replace |
3d85107
to
b0194a4
Compare
08df4c2
to
2e534e5
Compare
b593948
to
2691389
Compare
2691389
to
8ae89b6
Compare
Since we're never going to publish this, it should live in |
9ce843f
to
31286cb
Compare
Adds `tools/bench` allowing the benchmark of commands via the `ngbench` binary in development environments.
31286cb
to
49ca9dd
Compare
@hansl done. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds
@ngtools/benchmark
allowing the benchmark of commands via thengbench
binary.Total command running time is tracked, and specific matches within the command output can be added i.e. the
Time: 1443ms
report for individual builds.This is mostly a dev tool. Discussion is still needed to determine if it should be added.
Example usage:
ng serve
rebuild times (the two commands are equivalent).Fix #3606