Add iai adapter parser #98
Conversation
There was a problem hiding this comment.
Great start!
I marked this as Draft since it is still WIP.
I also changed the target branch to devel. Since this is the first real PR, it will make it easier to target devel given how I have CI set up. 😃
So you may want to do a merge or rebase from devel to clear up some of the minor diffs there.
| @@ -48,6 +55,31 @@ impl AdapterResults { | |||
| THROUGHPUT_RESOURCE_ID.clone() => json_metric | |||
| } | |||
| }, | |||
| AdapterMetricKind::Instructions(json_metric) => { | |||
There was a problem hiding this comment.
This helper function is for creating one-to-one benchmark name to metric kind relationships.
That is not what we want here. We want a one-to-many benchmark name to metric kind relation. That is one benchmark name with multiple metric kinds underneath/inside it.
This should be a separate helper function. Something like:
pub fn new_multi(benchmark_metrics: Vec<(BenchmarkName, HashSet<AdapterMultiMetricKind>)>) -> Option<Self>
There was a problem hiding this comment.
Sure, that sounds good. It seems like we'll have to change AdapterResults as well, since it can only represent a single benchmark_name -> result mapping. Does that sound good?
There was a problem hiding this comment.
We shouldn't have to change AdapterResults as it is a BenchmarkName -> AdapterMetrics and AdapterMetrics is MetricKind -> JsonMetric. So the data structure itself does not need to change. There just needs to be multiple keys in the AdapterMetrics map (instead of just one).
Hence the new helper function for AdapterResults. This will produce an AdaperResults containing an AdapterMetics that contains all of the Metric Kinds for Iai.
| @@ -26,6 +28,11 @@ impl From<ResultsMap> for AdapterResults { | |||
| pub enum AdapterMetricKind { | |||
| Latency(JsonMetric), | |||
| Throughput(JsonMetric), | |||
| Instructions(JsonMetric), | |||
There was a problem hiding this comment.
These should be split into their own enum (see comment below).
Something like: AdapterMultiMetricKind
| } | ||
|
|
||
| fn parse_iai_lines(lines: &[&str]) -> Option<(BenchmarkName, Vec<AdapterMetricKind>)> { | ||
| if lines.len() != 6 { |
There was a problem hiding this comment.
I get that this length check should make it impossible for the below indexing to panic, but I would prefer to use a single line pattern match instead.
let (Some(benchmark_name_line), Some(instructions_line), ...) = (lines.get(0), lines.get(1), ...) else {
return None;
}
| .expect("Failed to parse metric kind slug."), | ||
| ) | ||
| }); | ||
| pub const L1_ACCESSES_UNITS_STR: &str = "accesses"; |
There was a problem hiding this comment.
Maybe just have a single ACCESSES_UNITS_STR since they are all the same unit value.
Add unit test for iai parser
osiewicz
force-pushed
the
add_iai_adapter
branch
from
809fb9c to
fcd3cda
Compare
- Parse results as u64 instead of f64. - Do not accept JsonAverage::Mean from Settings. - Rewrite line unpacking in parse_iai_lines. - Use less verbose syntax for conversion to MetricKind. - Remove parse_from_header in favor of parsing the header directly in parse_iai_metric.
| .expect("Failed to parse metric kind slug."), | ||
| ) | ||
| }); | ||
| pub const L1_ACCESSES_UNITS_STR: &str = ACCESSES_UNITS_STR; |
There was a problem hiding this comment.
I don't think these aliases are necessary, just using ACCESSES_UNITS_STR below should be fine.
Split iai metrics into new enum; AdapterMultiMetricKind.
|
@osiewicz Thank you for all of work work here! |
This is still a bit WIP, as I intend to add new tests akin to those in criterion adapter.
(Partially?) solves #82