-
Notifications
You must be signed in to change notification settings - Fork 50
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
Validate execCPUAffinity
of the runtime process spec
#178
Conversation
a67075d
to
52804f0
Compare
@keisku thank you for the PR, do you mind a rebase? |
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
11635cd
to
18a85b2
Compare
@saschagrunert I did! |
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
@utam0k PTAL |
@@ -32,6 +32,8 @@ derive_builder = "0.20.0" | |||
getset = "0.1.1" | |||
strum = "0.26.2" | |||
strum_macros = "0.26.2" | |||
regex = "1.10.5" |
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'm worried about performance by introducing regex. WDYT? @saschagrunert
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 I agree, not sure I like that dependency right here.
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
@saschagrunert @utam0k Reimplemented the validation logic without |
@keisku Thanks, but apparently, the regex implementation is faster than the current implementation you updated. Depending on the length of the string, both are fast enough to merge. Sorry, can you revert to the regex implementation? $ cargo bench
simple_perf time: [99.829 ns 100.50 ns 101.24 ns]
change: [-0.6423% +0.6216% +1.7508%] (p = 0.33 > 0.05)
No change in performance detected.
Found 8 outliers among 100 measurements (8.00%)
7 (7.00%) high mild
1 (1.00%) high severe
regex_perf time: [24.250 ns 24.451 ns 24.669 ns]
change: [-2.0247% -0.9518% +0.1010%] (p = 0.08 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
4 (4.00%) high mild use criterion::{black_box, criterion_group, criterion_main, Criterion};
use regex::Regex;
fn regex_benchmark(c: &mut Criterion) {
c.bench_function("regex_perf", |b| {
let re = Regex::new(r"^(\d+(-\d+)?)(,\d+(-\d+)?)*$").unwrap();
b.iter(|| re.is_match(black_box("123,456-789,22,1,2,4,5,6")));
});
}
fn simple_benchmark(c: &mut Criterion) {
c.bench_function("simple_perf", |b| {
b.iter(|| {
let s = black_box("123,456-789,22,1,2,4,5,6");
let iter = s.split(',');
for part in iter {
let mut range_iter = part.split('-');
range_iter
.next()
.ok_or_else(|| format!("Invalid execCPUAffinity format: {}", s))
.unwrap();
// Check if there's a range and if the end is a valid number
if let Some(end) = range_iter.next() {
end.parse::<usize>().unwrap();
}
// Ensure there's no extra data after the end of a range
if range_iter.next().is_some() {
panic!("Invalid execCPUAffinity format: {}", s);
}
}
});
});
}
criterion_group!(benches, simple_benchmark, regex_benchmark);
criterion_main!(benches); |
This reverts commit 73c9740. Signed-off-by: keisku <keisuke.umegaki.630@gmail.com>
execCPUAffinity
, which was added in #174, expects a particular format.Ref: https://github.com/opencontainers/runtime-spec/blob/701738418b9555d5213337a0991fd0ffd6c37808/config.md?plain=1#L343-L353