perf: avoid cloning in filtering ptr #272
Conversation
CosminPerRam
changed the title
src/dns_cache.rs
Outdated
| .flatten() | ||
| .filter(|record| !record.get_record().is_expired(now)) | ||
| .flat_map(|record| record.any().downcast_ref::<DnsPointer>()) | ||
| .map(|ptr| &ptr.alias) |
There was a problem hiding this comment.
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)
})
src/dns_cache.rs
Outdated
| from: &'a HashMap<String, Vec<DnsRecordBox>>, | ||
| ty_domain: &str, | ||
| now: u64, | ||
| ) -> Vec<&'a String> { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
Line 395 in d117f4f
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.
CosminPerRam
changed the title
Extracts the filtering of instance names of a, the replaced code does not clone all the names, instead returns references to it (this is a perf improvement, as now they are being cloned only on demand (in checking the SRV records)) and I find this to be more concise and declarative, thoughts?ty_domainin a function as it was used in 2 places exactly the sameTests pass on MacOS 14.5.