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.

Sign up for GitHub

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

perf: avoid cloning in filtering ptr #272

Merged
merged 7 commits into from
Nov 24, 2024
Merged

perf: avoid cloning in filtering ptr #272

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
016be96
extract, no mut and return refs, not clones
CosminPerRam Nov 18, 2024
24ce727
fix filtering
CosminPerRam Nov 19, 2024
7c5aa42
rename parameter
CosminPerRam Nov 19, 2024
2fc86c6
chore: run cargo fmt
CosminPerRam Nov 19, 2024
ce228ea
chore: run cargo clippy
CosminPerRam Nov 19, 2024
b1231b9
restore extraction, use flat_map
CosminPerRam Nov 23, 2024
3b242ae
fix filtering
CosminPerRam Nov 23, 2024
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
46 changes: 19 additions & 27 deletions src/dns_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,64 +373,56 @@ impl DnsCache {
pub(crate) fn refresh_due_srv(&mut self, ty_domain: &str) -> (HashSet<String>, HashSet<u64>) {
let now = current_time_millis();

let mut instances = vec![];

// find instance names for ty_domain.
for record in self.ptr.get_mut(ty_domain).into_iter().flatten() {
if record.get_record().is_expired(now) {
continue;
}

if let Some(ptr) = record.any().downcast_ref::<DnsPointer>() {
instances.push(ptr.alias.clone());
}
}
let instances = Self::find_instance_names(&self.ptr, ty_domain, now);

// Check SRV records.
let mut refresh_due = HashSet::new();
let mut new_timers = HashSet::new();
for instance in instances {
let refresh_timers: HashSet<u64> = self
.srv
.get_mut(&instance)
.get_mut(instance)
.into_iter()
.flatten()
.filter_map(|record| record.updated_refresh_time(now))
.collect();

if !refresh_timers.is_empty() {
refresh_due.insert(instance);
refresh_due.insert(instance.clone());
new_timers.extend(refresh_timers);
}
}

(refresh_due, new_timers)
}

fn find_instance_names<'a>(
from: &'a HashMap<String, Vec<DnsRecordBox>>,
ty_domain: &str,
now: u64,
) -> Vec<&'a String> {
Copy link
Owner

@keepsimple1 keepsimple1 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this definition of a function seems arbitrary. I would opt for just changing the imperative style to the functional style in each place. (The apparent duplication is OK for such functional style, and it will save two function calls).

(also, if we did use a function, it would be better to take &self instead from, and use self.ptr inside the function).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using &self instead of from would return a reference that would be tied to the entire self, and we cant do that without cloning the entire vector because we have a mutable reference here:

mdns-sd/src/dns_cache.rs

Line 395 in d117f4f

.get_mut(&instance)

So, instead, as you also mentioned, I reverted the extraction, and by such, this PR is just a perf improvement as it avoids many .clone calls.

from.get(ty_domain)
.into_iter()
.flatten()
.filter(|record| !record.get_record().is_expired(now))
.flat_map(|record| record.any().downcast_ref::<DnsPointer>())
.map(|ptr| &ptr.alias)
Copy link
Owner

Choose a reason for hiding this comment

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

IMO, flat_map is harder to understand than filter_map, hence I think probably combine flat_map and map into the following, would be a bit easier to undertand:

    .filter_map(|record| {
        record.any()
            .downcast_ref::<DnsPointer>()
            .map(|ptr| &ptr.alias)
    })

.collect()
}

/// Returns the set of `host`, where refreshing the A / AAAA records is due
/// for a `ty_domain`.
pub(crate) fn refresh_due_hosts(&mut self, ty_domain: &str) -> (HashSet<String>, HashSet<u64>) {
let now = current_time_millis();

let mut instances = vec![];

// find instance names for ty_domain.
for record in self.ptr.get_mut(ty_domain).into_iter().flatten() {
if record.get_record().is_expired(now) {
continue;
}

if let Some(ptr) = record.any().downcast_ref::<DnsPointer>() {
instances.push(ptr.alias.clone());
}
}
let instances = Self::find_instance_names(&self.ptr, ty_domain, now);

// Collect hostnames we have browsers for by SRV records.
let mut hostnames_browsed = HashSet::new();
for instance in instances {
let hosts: HashSet<String> = self
.srv
.get(&instance)
.get(instance)
.into_iter()
.flatten()
.filter_map(|record| {
Expand Down