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

Removal of smallvec from dependancies. #1974

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darnuria
Copy link
Contributor

@darnuria darnuria commented Jun 20, 2023

Hello, taking interrest with issue: #365

Disclamer: the approach is naive I know it involve a most to malloc and leaved the code untouched even allocating in a loop, mostly to began exploring the subject! :)

In crate/resolver I chosed to use a plain vec since the type: SmallVec::<[Nameservers<P>; 2] reached 544 bytes not sure it's small anymore. (did a little std::mem::size_of::<SmallVec<[NameServer<P>;2]>>() to get the size in bytes)

Why I didn't used TinyVec directly? Because it would have needed a Default implementation on each types of Nameservers.

If it's not the best course of action I am ok to get some mentoring! :)

Some idea I have in mind:

  • If it's not the right course of action: either close Evaluate all Vec usage and replace with SmallVec where apropriate #365, or reformulate it and document here why smallVec is mandatory here.
  • If it's OK but need tinyVec, will need some work since Default is mandator due to TinyVec Array trait.
  • If the plain Vec is good enough then just have something less naive ! ;)

In crate/resolver I choosed to use a plain vec since the
type: SmallVec::<[Nameservers<P>; 2] reached 544 bytes not sure
it's small anymore.
@darnuria darnuria changed the title Removal of smallvec from depenancies. Removal of smallvec from dependancies. Jun 20, 2023
@bluejekyll
Copy link
Member

Sorry for the late response here. This looks really good to me. I like simplifying the dependency graph, and SmallVec is probably not buying much here. For reference, the idea here was that since the vast majority of use cases would probably not override this setting (2 NameServers for lookup) then it would in theory be more efficient to store a fixed size for that use case. This might not be well-founded, and as you already identified, may not actually save us much in terms of allocation.

@bluejekyll
Copy link
Member

Do you mind if I update this branch for you? or would you like to rebase?

@bluejekyll
Copy link
Member

bluejekyll commented Jul 27, 2023

@darnuria, I just noticed this was a draft, feel free to turn this into a PR ready for review, I'll approve and merge.

@darnuria
Copy link
Contributor Author

darnuria commented Jul 27, 2023 via email

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.

Evaluate all Vec usage and replace with SmallVec where apropriate
2 participants