Conversation
| } | ||
|
|
||
| @Override | ||
| public void setPrintPrecision(String fmt) { |
There was a problem hiding this comment.
Can we return a QueryBenchmark here for chaining? See other comments.
There was a problem hiding this comment.
Add return type QueryBenchmark.
return this;
|
|
||
| double avgRuntimeSec = totalRuntime / queryRuns / 1e9; | ||
| return new Summary(avgRuntimeSec); | ||
| return List.of(Metric.of("QPS", getPrintPrecision(), avgRuntimeSec)); |
There was a problem hiding this comment.
The string should be "Avg Runtime (s)"
| int queryRuns | ||
| ); | ||
|
|
||
| void setPrintPrecision(String fmt); |
There was a problem hiding this comment.
If you like the idea of chaining setPrintPrecision() in Grid, we just need a return type of QueryBenchmark here.
| System.out.format("Using %s:%n", cs.index); | ||
| // 1) Select benchmarks to run | ||
| var benchmarks = List.of( | ||
| List<QueryBenchmark> benchmarks = List.of( |
There was a problem hiding this comment.
Would like to be able to chain the .setPrintPrecision on any of these benchmarks here when instantiated. See two other comments on this.
jvector-examples/src/main/java/io/github/jbellis/jvector/example/Grid.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void setPrintPrecision(String fmt) { |
There was a problem hiding this comment.
I like the way you are suggesting we set and get the precision. The only drawback I see is that it was previously set at the value level within a benchmark. That way, it could be set individually per value, not globally per benchmark. But not a big deal if each benchmark limits its produced values to ones sharing the same units and approx ranges of values.
tlwillke
left a comment
tlwillke
left a comment
There was a problem hiding this comment.
Good changes. I think the constructor overloading for print formats fell into the telescoping constructor anti-pattern. And yet, some useful overloads are still missing. I think I prefer a chaining design which is more readable and only specifies what's relevant:
LatencyBenchmark()
.displayAvgLatency("0.2f") // Non-default formatting on the default column
.displayP999Latency(); // An optional column with default formatting
But the current version is good enough for now!
* Improve configuration of benchmarking suite * Allow to set the precision format and other minor improvements. * Use minimal table by default * Fix javadoc * Fix javadoc * Add missing license * Enable richer setup of the printing format for each metric * Reorder the default table columns
2 participants
This PR extends #459 in a few simple ways: