keepsimple1 / mdns-sd

Rust library for mDNS based Service Discovery
Apache License 2.0
102 stars 39 forks source link

perf: in adding answers, use static dispatch instead of dynamic dispatch #207

Closed CosminPerRam closed 6 months ago

CosminPerRam commented 6 months ago

This is an alternative to #206, changes the answer type from Box<dyn DnsRecordExt> to impl DnsRecordExt + Send + 'static.

This replaces the usage of dynamic dispatch on every function call with static dispatch, which in our case should be faster as we always call these functions with new instances (Box::new(...)), so the compiler should be able to monomorphize and/or optimize further its usage, as all the possible implementations are now known at compile time.

The dynamic dispatch is still needed for the answer field (Vec<(Box<dyn DnsRecordExt>, u64)>) in self.answers.push of add_answer_at_time, but this is done only if needed (when if conditions are met), so this should provide us with only performance improvements, please note that I'm not really a Rust expert or anything, nor have I done benchmarks to provide this (maybe we could add some eventually?).

Edit: forgot to mention that the disadvantages of static dispatch are longer compile times and larger binary size (but I haven't seen significant changes in these)

keepsimple1 commented 6 months ago

On the grand scheme, I think the performance difference is small / not significant for this lib, unless we have some measurement data showing a problem. But It's a learning opportunity for me to understand more about dynamic dispatch and static dispatch in Rust.

This replaces the usage of dynamic dispatch on every function call with static dispatch,

For my understanding, do you have a pointer to Rust doc or something similar to verify that? I don't know for sure, and asked ChatGPT, and here is what it says:

...even with the impl Trait syntax, dynamic dispatch will still be used when calling the add_additional_answer function because answer is still treated as a trait object. ...

Is ChatGPT hallucinating here?

CosminPerRam commented 6 months ago

Is ChatGPT hallucinating here?

Asking ChatGPT (4) about this says that this statement is incorrect, I think it says that it still implies dynamic dispatch as we eventually use .push(Box::new(answer)).

Here is how Brandeis University explains what is static and dynamic dispatch and their advantages and disadvantages. Here is a shorter example and explanation.

I see the benefit of not requiring the callers to add Box::new() at the call site. So I'm okay with such change, for ease of use, not for the performance reason.

Fair enough, if I will get the time I'll try to make some benchmarks just as a curiosity (: