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

Remove need for Polonius borrow checker in git_odb::compound::Db::locate(…) #66

Closed
Byron opened this issue Apr 7, 2021 · 1 comment

Comments

@Byron
Copy link
Owner

Byron commented Apr 7, 2021

Currently there is branching code in git_odb::compound::Db::locate(…) to workaround a borrowcheck shortcoming using the upcoming Polonius borrow checker.

// See 8c5bd095539042d7db0e611460803cdbf172beb0 for a commit that adds polonius and makes the proper version compile.
// See https://stackoverflow.com/questions/63906425/nll-limitation-how-to-work-around-cannot-borrow-buf-as-mutable-more-than?noredirect=1#comment113007288_63906425
// More see below! Of course we don't want to do the lookup twice… but have to until this is fixed or we compile nightly.
#[cfg(not(feature = "polonius"))]
if alternate.locate(id, buffer)?.is_some() {
return alternate.locate(id, buffer);
}
#[cfg(feature = "polonius")]
if let Some(object) = alternate.locate(id, buffer)? {
return Ok(Some(object));
}

Without the workaround, the lookup costs are doubled.

Using Polonius solves the problem but comes at a cost.

  • It requires the nightly compiler (forcing gitoxide to be compiled with nightly if that code-path is chosen).
  • The borrow checker performance is reduced and can 'explode' for certain crates like tinyvec.
  • It's unclear when Polonius will be stable.

Here is @joshtriplett thoughts on how to resolve this:

When first initializing the Db and looking up all the alternates, rather than recursing, collect all of the alternates as a top-level Vec of (Packs, Loose), so that alternates themselves never have alternates. That way, you don't have to recurse, and there's only one level of alternates. Then, in the loop over alternates, rather than recursively calling locate, just inline the same code that looks in packs and loose, but without the check for alternates.
I think the simplest way to do that would be to have a version of at (at_no_alternates) that ignores alternates entirely, and then have the top-level at collect a deduplicated list of alternates and call at_no_alternates on each one.

Additional Information

Also have a look at how this is currently done for looking up objects in packs. The gist is to get an index of where the object is found first, bypassing the need for borrowing an output buffer, and fill the buffer if an index was found.

if let Some(object) = self.loose.locate(id)? {
return Ok(Some(compound::Object::Loose(object)));
}

@Byron Byron added this to To do in Collaboration Board Apr 7, 2021
@Byron Byron mentioned this issue Apr 10, 2021
43 tasks
@Byron
Copy link
Owner Author

Byron commented Apr 13, 2021

  • linked repo type with init
  • linked repo locate()
  • support for caches in locate()
  • object-access experiment uses new repo
  • docs

@Byron Byron added this to In progress in Open Source Work Apr 13, 2021
@Byron Byron closed this as completed Apr 13, 2021
Open Source Work automation moved this from In progress to Done Apr 13, 2021
Collaboration Board automation moved this from To do to Done Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

1 participant