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

Simplified and more bound-friendly IntoLender; Lend help type #3

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

vigna
Copy link
Collaborator

@vigna vigna commented Oct 23, 2023

We are trying to use this crate instead of our own-made lending iterator crate for the Rust port of WebGraph and we ran into a problem.

The way IntoLender is defined requires to specify, again, the type of item lent via a dependency from Lending. So there are now two specifications of the same thing—one in IntoLender, the other one in IntoLender::Lender.

Because of this, we haven't been able to make our methods that receive IntoLender work (they have quite complicated trait bounds). Depending on whether we specify the bounds on IntoLender or on the associated Lender we get different problems, and also specifying both bounds doesn't seem to work.

However, it seems that all that is necessary is to get rid of the dependency of IntoLender from Lending. All trait bounds for the lent item for L: IntoLender can be equivalently rewritten using L::Lender. This reduces the burden on implementors (no need to implement Lending for IntoLender implementations), and is equivalent to specifying twice the requirement in the same way. The change should also be backward-compatible, as existing implementations of Lending for IntoLender implementations will just be useless, but not harming. (Ok, if someone has implemented a method receiving an IntoLender L and has written trait bounds using L as Lending that might require to replace L with L::Lender.)

Finally, this PR adds (similary to our crate) a Lend type

pub type Lend<'lend, L> = <L as lender::Lending<'lend>>::Lend;

We have a similar alias in our crate and it really helps in making trait bounds readable.

We can of course work with a fork, but we really like this crate and it would be amazing if we could make it work with our code.

Copy link
Owner

@WanderLanz WanderLanz left a comment

Choose a reason for hiding this comment

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

looks good! I somewhat recall having the awkward IntoLender syntax because of some errors in edge cases, but I can neither remember what they were nor can I find them again, so maybe it was just the rustc version I was using.

Fair warning, the crate is as complete as I have the willpower or time to make it at the moment, so I won't be actively updating it. However, I do appreciate the usage and contributions. I may not be available, so if you would like to be added as a maintainer, let me know.

@WanderLanz WanderLanz merged commit b1d3db9 into WanderLanz:main Oct 24, 2023
1 check passed
@vigna vigna deleted the simple_into branch October 24, 2023 09:23
@vigna
Copy link
Collaborator Author

vigna commented Oct 24, 2023

Yes, that would be great. Of course under your supervision :).

We have another small problem we're trying to solve. The map method actually maps a Lender into a Lender, but the problem is that requires the hrc_mut macro, which in turn requires to write down the type explicitly (at least, I couldn't find a way to make type inference work). The type cannot be written explicitly because, among other things, depend on a type parameter of the hosting method, and the compiler complains if you use it in a function.

Our original implementation naively implemented map in a way that would return a non-lending iterator. We didn't realize that until we found your crate and Sabrina's blog entry.

However, we realize now that it could be actually a feature. There could be a map_into_iter method that takes a normal function, without the higher-order macro, and returns a standard Iterator mapping the values to owned types. It is a very frequent use case, because usually you do something on your lent data but then produce owned data out of that (e.g., statistics). Such a method would not need to go through all the higher-rank stuff (see https:/vigna/hrtb-lending-iterator-rs/blob/main/src/adapters/map.rs) and could even, effectively, return a standard Iterator (but the link above doesn't).

@WanderLanz
Copy link
Owner

@vigna That does seem like a common enough case to provide better ergonomics for, sounds great.

Looking at the usage of the more popular lending-iterator crates, I don't think this crate will find much usage outside of very niche projects either, so I don't have a problem with adding ergonomics where you and your team would find personally helpful. For all intents and purposes, consider you and your team to have "community consent" in adding whatever you'd like.

Hope you have a good day!

@vigna
Copy link
Collaborator Author

vigna commented Oct 24, 2023

Look, we're examined the options around and yours is by far the most complete and flexible crate for this kind of iterators. I think a lot of people does not even realize you can write lending iterators in Rust—just understanding that was for us a slow process, and even talking to expert there was a constant ping-ping of playground to get all the features we needed working. The point is that lending is an efficiency issue, not a semantics issue, so unless you are working on massive data sets, like we do, tight on memory, like we do, you might simply use a standard iterator with a lot of copying because it will work for you. But iterators returning items depending on the state of the iterator are a fundamental construct. I didn't realized their intricacies until I had to write them the Rust—in other languages, you just try to not shoot yourself in the foot.

@WanderLanz
Copy link
Owner

Thank you for the compliment, Rust's std::iter has quickly become a favorite of mine among iterator APIs and hopefully I've emulated its flexibility and ergonomics enough. I'm happy this crate has been helpful to your team.

On iterator efficiency and performance:
Some problems with iterators come from the underlying LLVM itself, and Rust would need to make iterators a compiler built-in to achieve hand-written efficiency and performance, similar to Haskell. As an unfortunate result, in extremely performance-conscience contexts, it's usually better to keep iterators as simple as possible in both Rust and C++. Rust does have some internal tricks in std::iter to optimize for the common case (compiler directives, TrustedLen, etc.) but it does certainly fall short outside of the common case when you are struggling for efficiency or performance.

I don't have any experience with Webgraphs, but it sounds like your project is daunting, I wish you happy engineering and easy debugging.

@vigna
Copy link
Collaborator Author

vigna commented Oct 25, 2023

Could you kindly make me also an owner of the crate? In this way I can publish new versions. Or, if you want maintain control, could you pull and publish 0.2.0? In this way we can immediately start to use the library. I bumped the second digit because there might be incompatibilities with the new IntoLender.

@WanderLanz
Copy link
Owner

WanderLanz commented Oct 25, 2023

Done! The crate is still experimental, so the version number doesn't need to be strict, but there's no problem following standard.

Just a heads up, there is a pull request #115538 that may fix #84533 in Rust which might break some non-essential functionality (anything which can use lend!()), so I think at least v1.0 should probably wait until Rustlang has consensus on that. We are using core language design flaws to make everything work after all 😂.

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.

2 participants