-
-
Notifications
You must be signed in to change notification settings - Fork 55
Benchmark runner (WIP) #143
Benchmark runner (WIP) #143
Conversation
@mistakia fantastic! 👍❤️ I really like the approach you've taken to create a benchmark runner. It'll be much easier to add more benchmarks in the future as well as have coherent api/code for all the benchmarks. It would be great if we can specify how long each benchmark in run, both as in "how many cycles to run" as well as "how many second". The latter is hugely important to be able to let the benchmark run for a long period of time to observe behaviour (algorithmic complexity, memory usage, stress testing) over a long period of time while the first is great in getting a "stable" benchmark baseline that can be compared, for example in CI, against previous versions. What do you think? |
Agreed. As the PR currently stands that would be accomplished by two different tests like so: {
name: 'append-baseline',
...
while: (stats, startTime) => {
return stats.count < 1000 // 1000 iterations
},
...
} {
name: 'append-stress',
...
while: (stats, startTime) => {
return process.hrtime(startTime)[0] < 900 // 15 mins
},
...
} I can create two groups of tests: Let me know if thats acceptable (and if you have better names for the groups) and I can update the PR accordingly. |
24b7539
to
da978be
Compare
Hi @mistakia, how are you? I'm the new guy on the Haja networks team and I'm reaching out to see if I can help with this PR in any way? |
@aphelionz nice to meet ya! Just getting back into this and catching up on all the changes in ipfs/orbit-db/ipfs-log. Once I have this rebased to master, I'll have a better idea of what help/questions I may need/have to get this to the finish line. |
@mistakia Great! I'm here to help. Let's get this over the finish line :) |
8520e96
to
558e29d
Compare
@aphelionz I think I'm caught up — rebased, updated (linting + new constructor) and added the remaining benchmarks. It's nearly ready for review after addressing the following: (1) I would like to add a feature where CircleCI can compare PR benchmarks to master by committing a file to the repo that the benchmark runner can use to compare. Let me know if there's a better approach and/or if this is a desired feature. (2) (3) |
0e5ef13
to
3d98d8d
Compare
seeing some issues related to ipfs/js-ipfs-repo/issues/188 |
3d98d8d
to
8cca728
Compare
2a0d334
to
f8699f1
Compare
Rebased & fixed memory measurement issue. I think this is ready for review. Having to run with node v11 to avoid alanshaw/pull-stream-to-async-iterator#1 Let me how to proceed. |
can now run stress benchmarks for any time limit by passing in a limit (in seconds or Infinity)
|
This looks great @mistakia ! I'd love a quick primer on how to use this, either in a README or another MD file, and then I'd love to see it merged. What I really want to see in the end is this incorporated into CI so developers can see the impact their work has on performance with each PR. |
Cool! Playing with it now and having a lot of fun. THANK YOU so much for doing this. Here's stuff I'd love to see:
All that being said, this is fine to merge as is and we can move the above list to an issue to tackle asynchronously from this. I actually really want to use this ASAP to sink my teeth into the append-performance branch, and then start using this as a paradigm for the various |
Sorry, not "Finally" :)
|
I should have some time in the next few days to tackle those issues. Most of it should be quick and straightforward. Only thing that requires some thinking is incorporating it into CI. If that’s not quick enough, feel free to merge because I can’t really think of a great reason to wait. As for the CI integration, my thinking was adding a flag to the command that will write the results to a json file that can be committed into the repo. If such a file exists, the benchmark reporter will use it to generate a comparison. Then we can just add the benchmark runner/reporter to CI. Let me know if you have a better approach. |
558dd68
to
d02c85f
Compare
I've updated the PR with the following changes:
I'm working on integrating with CircleCI but am struggling a bit to plot a path forward. My previous suggestion doesn't quite make sense as the saved benchmark file that is committed to the repo is run on a different machine and thus is not a good comparison when used in CircleCI. I'm not very familiar with CircleCI yet but it appears I can save test metadata as "artifacts" but I'm not sure how I would access it later inside a job. The build/test job of a pr branch will have to access the metadata generated from the last master build/test job. |
Sounds good. Let me know how I can help. Also, I should note that this PR adds yargs as a dev dep. |
This is fantastic, thank you @mistakia! ❤️ |
Awesome work @mistakia and @aphelionz 👏 This is a really great addition to the dev tooling, LGTM 👍 |
Congrats @mistakia :) |
Broke ground on a barebones benchmark runner based on the benchmarks that currently exist.
I was thinking about having benchmarks that capture average ops/second over a set time span (
10s
?) and use various log sizes (1
,100
,1000
,10000
,50000
) for reads and certain static methods.Let me know if you have a better approach to
fromEntryHash
,fromJSON
, etc where ops/second are not as meaningful for larger log sizes.Then we can cache the results (via committing a file to the repo? or maybe on circle somehow) so that circle CI can run and compare PRs to master.
Let me know what you think about the overall approach — then I can round out this PR to separate the
benchmarks.js
file, add all the benchmark tests for the public & static methods, and setup fancy logging.Todo
Benchmarks
findTails (duplicate of tails?)findTailHashes (duplicate of tailHashes?)Ref: #140