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

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

Merged

Conversation

CosminPerRam
Copy link
Contributor

@CosminPerRam CosminPerRam commented May 1, 2024

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)

@@ -158,7 +158,7 @@ impl PartialEq for DnsRecord {
}
}

pub trait DnsRecordExt: fmt::Debug {
pub trait DnsRecordExt: fmt::Debug + Send {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler automatically implements Send on appropriate types, but as we have only usages of this now, it's a little bit more concise to explicitly state it on the trait rather than on the Box (and not in the answer functions), theoretically at least, should be the same thing but I think stating it like this rather than how it was is a bit better, what do you think?

Copy link
Owner

@keepsimple1 keepsimple1 May 3, 2024

Choose a reason for hiding this comment

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

I think it is odd to have DnsRecordExt trait to conform Send, because this trait is really just about supporting different types of DNS records, nothing related to dispatching mechanisms (e.g. Send or Sync, etc).

I can understand that moving Send to the trait would simplify the parameter signature in functions, but it doesn't feel the right reason for me.

@keepsimple1
Copy link
Owner

keepsimple1 commented May 3, 2024

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?

@@ -771,14 +771,18 @@ impl DnsOutgoing {
// server/responder SHOULD include the following additional records:

// o All address records (type "A" and "AAAA") named in the SRV rdata.
pub(crate) fn add_additional_answer(&mut self, answer: DnsRecordBox) {
pub(crate) fn add_additional_answer(&mut self, answer: impl DnsRecordExt + 'static) {
Copy link
Owner

@keepsimple1 keepsimple1 May 3, 2024

Choose a reason for hiding this comment

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

Although I'm not sure about the static dispatch thing, 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.

And I think I prefer to have Send in here (if needed), compared to move Send into DnsRecordExt trait, as I mentioned in another comment.

@CosminPerRam
Copy link
Contributor Author

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 (:

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@keepsimple1 keepsimple1 merged commit bf5cea3 into keepsimple1:main May 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants