Skip to content
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

chore: Speed up LogEvent::rename_key by 57% #9226

Merged
merged 1 commit into from
Sep 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/vector-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,7 @@ vrl = ["vrl-core", "enrichment"]
[[bench]]
name = "lookup"
harness = false

[[bench]]
name = "event"
harness = false
61 changes: 61 additions & 0 deletions lib/vector-core/benches/event/log_event.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
use criterion::{
criterion_group, measurement::WallTime, BatchSize, BenchmarkGroup, Criterion, SamplingMode,
};
use std::time::Duration;
use vector_core::event::LogEvent;

fn rename_key_flat(c: &mut Criterion) {
let mut group: BenchmarkGroup<WallTime> =
c.benchmark_group("vector_core::event::log_event::LogEvent::rename_key_flat");
group.sampling_mode(SamplingMode::Auto);

group.bench_function("rename_flat_key (key is present)", move |b| {
b.iter_batched(
|| {
let mut log_event = LogEvent::default();
log_event.insert("one", 1);
log_event.insert("two", 2);
log_event.insert("three", 3);
log_event
},
|mut log_event| {
log_event.rename_key_flat("one", "1");
},
BatchSize::SmallInput,
)
});

group.bench_function("rename_flat_key (key is NOT present)", move |b| {
b.iter_batched(
|| {
let mut log_event = LogEvent::default();
log_event.insert("one", 1);
log_event.insert("two", 2);
log_event.insert("three", 3);
log_event
},
|mut log_event| {
log_event.rename_key_flat("four", "4");
},
BatchSize::SmallInput,
)
});
}

criterion_group!(
name = benches;
config = Criterion::default()
.warm_up_time(Duration::from_secs(5))
.measurement_time(Duration::from_secs(120))
// degree of noise to ignore in measurements, here 1%
.noise_threshold(0.01)
// likelihood of noise registering as difference, here 5%
.significance_level(0.05)
// likelihood of capturing the true runtime, here 95%
.confidence_level(0.95)
// total number of bootstrap resamples, higher is less noisy but slower
.nresamples(100_000)
// total samples to collect within the set measurement time
.sample_size(150);
targets = rename_key_flat
);
5 changes: 5 additions & 0 deletions lib/vector-core/benches/event/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
use criterion::criterion_main;

mod log_event;

criterion_main!(log_event::benches);
7 changes: 6 additions & 1 deletion lib/vector-core/src/event/log_event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,17 @@ impl LogEvent {
K: AsRef<str> + Into<String> + PartialEq + Display,
{
if from_key != to_key {
if let Some(val) = self.remove(from_key) {
if let Some(val) = self.fields.as_map_mut().remove(from_key.as_ref()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the tests below to catch this in the future? Presumably self.remove() was not right since it would have interpreted the key as a path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long-term I'd prefer to see the _flat functions disappear. Right now we avoid them to get a performance boost while the two lookup situations are in play. I do see your point about checking that we don't interpret the path, though. I'd like to gate that work on #9131 since it'll be a natural consequence of doing that work well.

self.insert_flat(to_key, val);
}
}
}

/// Insert a key in place without reference to pathing
///
/// This function will insert a key in place without reference to any
/// pathing information in the key. It will insert over the top of any value
/// that exists in the map already.
#[instrument(level = "trace", skip(self, key), fields(key = %key))]
pub fn insert_flat<K, V>(&mut self, key: K, value: V)
where
Expand Down