-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
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.
Looks great overall - some small changes that we discussed elsewhere. React review coming, and would be great to get @mistakia's thumbs up as well
Still need test and docs too...
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.
LGTM
The approach and code LGTM. Doing some testing using the ipfs-log and orbit-db benchmarks, will test it out by evaluating orbitdb-archive/ipfs-log#301 |
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.
One more question here and I think we'll be good to go
@@ -4,16 +4,43 @@ | |||
"description": "OrbitDB Benchmark Runner", | |||
"main": "./src/index.js", | |||
"bin": { | |||
"benchmark-runner": "./src/cli.js" | |||
"benchmarker": "./src/cli.js" |
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.
Should we change the name of the repo to orbit-db-benchmarker
? Thinking about consistency.
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.
this could be changed to anything or kept the same, i chose the different name originally so it didnt conflict. probably good to have the package and repo the same name though (they are different right now; repo: benchmark-runner, package: orbit-db-benchmark-runner). shorter name probably better. also keep in mind the benchmarker doesnt have anything written in that makes it specific to benchmarking orbitdb, it can run benchmarks for anything.
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.
Alright let's just leave it as is for now until it becomes a real problem
largish pr, apologies.
the changes are primarily
much of the original source has been overwritten and restructured:
For each benchmark in dir/file path given,
cli.js
runsexec-benchmark.js
inside of a nodejs Worker.exec-benchmark.js
importsexec-benchmark.node.js
orexec-benchmark.browser.js
to run the benchmark.exec-benchmark.node.js
is for running the benchmark in node, it only exportsrun.js
which is what actually runs the benchmarks.exec-benchmark.browser.js
first bundles the benchmarks andrun.js
to be ran in the browser and hosts it on a webpack dev server (seewebpack-server.js
andwebpack-entry.js
). once the bundle is ready it visits the page with puppeteer and waits for the benchmarks to finish.In both environments the benchmarks are recorded with
benchmarker/server.js
&benchmarker/client.js
.once benchmarks have been ran the results are given to
reporter/index.js
to generate a report. First the results are processed and then a report is printed to console. Optionally with a cli flag the reporter can write a json or html report to a given path.JSON reports with another cli flag can be used to compare a new benchmark.
HTML reports show the metrics and some graphs of the metrics.
benchmark file structure has been changed. a benchmark file must export an object with a method named benchmark and benchmark file names should end in
.benchmark.js
. here are some examples: