services/blob/src/database/types.rs
153 ↗	(On Diff #31787)	Do we need both `Eq` and `PartialEq`?
services/blob/src/service.rs
313–323 ↗	(On Diff #31787)	It is something similar to `partition`, but I guess it might be challenging to use it when `self` is a vector.

This revision is now accepted and ready to land.Oct 10 2023, 5:06 AM

LGTM, potentially we could add these optimisations:

Return impl Iterator<> instead of Vec
Return &'self str in blobs_to_find_holders

but I haven't analysed it very thoroughly so there might be lifetime/ergonomics issues

In D9351#276883, @michal wrote:

LGTM, potentially we could add these optimisations:

Return impl Iterator<> instead of Vec

Thanks! I'll see if this saves me one unnecessary collect() into Vec

Return &'self str in blobs_to_find_holders

I'll look into this. Anyway, cloning small strings is cheap (holder/blob-hash are usually no longer than 40-50 characters)

services/blob/src/service.rs
313–323 ↗	(On Diff #31787)	The `partition` way might be not the most efficient way. Bear in mind that `self` is a `BTreeMap` while `checked` is a `Vec`.

bartek added inline comments.Oct 10 2023, 8:25 AM

services/blob/src/database/types.rs
153 ↗	(On Diff #31787)	Yes, they are needed for `HashMap` / `BTreeMap` keys

In D9351#276883, @michal wrote:

Return impl Iterator<> instead of Vec

Tried that, but unfortunately these methods are already inside a trait, and it turns out that "return impl trait inside a trait" is unstable 😞 (see also this SO answer)

│  impl Trait only allowed in function and inherent method return types, not in trait method return types rustc (E0562) [500, 39]
│ see issue #91611 https://github.com/rust-lang/rust/issues/91611 for more information

Doing fn foo<T: Iterator<Item = ...>>(&self) -> T; doesn't work either