Page MenuHomePhabricator

[blob] Implement cleanup helper functions
ClosedPublic

Authored by bartek on Oct 4 2023, 3:04 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 14, 2:44 AM
Unknown Object (File)
Thu, Jun 13, 1:27 AM
Unknown Object (File)
Wed, Jun 12, 6:15 PM
Unknown Object (File)
Wed, Jun 12, 1:58 PM
Unknown Object (File)
Sat, May 25, 7:20 PM
Unknown Object (File)
May 4 2024, 6:23 PM
Unknown Object (File)
May 4 2024, 6:23 PM
Unknown Object (File)
May 4 2024, 6:23 PM
Subscribers

Details

Summary

Implementation of helpers introduced in D9350. These are part of the cleanup logic
and are extensively used in the next diff

Depends on D9350

Test Plan

The code compiles, these are tested thoroughly in the next diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Oct 4 2023, 3:21 AM

Rebase. Fix filter-map condition

tomek added inline comments.
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:

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

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

LGTM, potentially we could add these optimisations:

  1. Return impl Iterator<> instead of Vec

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

  1. 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.

services/blob/src/database/types.rs
153 ↗(On Diff #31787)

Yes, they are needed for HashMap / BTreeMap keys

  1. 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

Rebase on parent diff feedback

This revision was automatically updated to reflect the committed changes.