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

Return impl iterator from permutate methods #19

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

sd2k
Copy link
Contributor

@sd2k sd2k commented Oct 26, 2020

Hi!

Not sure if you'd be interested in this change but it fit my use case slightly better and thought you may find it useful - feel free to close this if not.

It removes the Boxing and fallibility on all the various permutation methods of Domain to simplify the API slightly and (hopefully) improve memory usage, since there are no internal vecs, just iterators.

I think some of these methods may actually be slightly slower than before, although I'm not sure how, and the difference is a few microseconds here and there. I have a separate branch with benchmarks which I'll push so you can use them and see for yourself if you like.

…bject

Also remove many of the intermediate vecs used in the various methods. I
don't know if this is any faster but it feels more idiomatic, and should
reduce memory usage when scanning lots of domains, since intermediate
allocations should be reduced.

This is obviously a breaking change since it changes the public API
considerably.
@JuxhinDB
Copy link
Collaborator

First of all, thanks a bunch for the PR @sd2k -- it's definitely very welcome! I don't have particular objections to any of these, but I'll go through them slowly before I merge. :-)

Out of curiosity, may I ask what your use-case is? If you prefer to keep it anonymised, you can just reach out to me on twitter (same handle).

@JuxhinDB JuxhinDB self-assigned this Oct 26, 2020
@JuxhinDB JuxhinDB added the enhancement New feature or request label Oct 26, 2020
@sd2k
Copy link
Contributor Author

sd2k commented Oct 26, 2020

First of all, thanks a bunch for the PR @sd2k -- it's definitely very welcome! I don't have particular objections to any of these, but I'll go through them slowly before I merge. :-)

Cool, glad to hear it! Let me know if you have any concerns. One thing to note would be that returning impl Trait in a public API is sometimes not the best option (there's a discussion here) but otherwise we'd need to name an iterator struct for every method on Domain I believe. Perhaps that would be a better way forward for a future version?

Out of curiosity, may I ask what your use-case is? If you prefer to keep it anonymised, you can just reach out to me on twitter (same handle).

Nothing too exciting really; at $WORK we're looking into ways to investigate lots of domains and I thought it would be fun to try it in Rust instead, but ideally with as low a footprint as possible 🙂

@JuxhinDB
Copy link
Collaborator

Perhaps that would be a better way forward for a future version?

As it is right now, it improves the interface and implementation drastically, so I am going to roll with it for the time being and push out a 0.3.x-beta version with this change. As we head closer to a stable release the entire interface will be evaluated more closely with hopefully some more real-world use-cases.

Nothing too exciting really; at $WORK we're looking into ways to investigate lots of domains and I thought it would be fun to try it in Rust instead, but ideally with as low a footprint as possible

I'm glad to hear that, I hope it works out!


Domain::filter_domains(Box::new(result.into_iter()))
pub fn replacement(&self) -> impl Iterator<Item = String> + '_ {
Self::filter_domains(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sd2k is there a particular reason you opted for Self::filter_domains here or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, not here specifically, no. Usually I would use Self::filter_domains everywhere (I think that's recommended by clippy sometimes? perhaps on a pedantic link), perhaps in case the struct name ever changes - I can't remember why I started tbh!

@JuxhinDB JuxhinDB merged commit 9c7eaba into haveibeensquatted:master Oct 26, 2020
@JuxhinDB
Copy link
Collaborator

Version has been bumped. You should be able to pull 0.3.0-beta now :-)

@sd2k sd2k deleted the return-impl-iterator branch October 26, 2020 22:17
@sd2k
Copy link
Contributor Author

sd2k commented Oct 26, 2020

Awesome, thanks @JuxhinDB! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants