Refactor EvictingMap so it does not use DigestInfo

[allada]

In prep to move to an abstract key instead of DigestInfo, this change cleans up EvictingMap and removes dependencies on DigestInfo.

towards: #931

---

This change is 

[allada]

+@adam-singer or +@zbirenbaum

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, vale (waiting on @adam-singer and @zbirenbaum)

[zbirenbaum]

Reviewable status: 0 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @adam-singer)

---

nativelink-util/src/evicting_map.rs line 194 at r1 (raw file):

    }

    pub async fn build_lru_index(&self) -> SerializedLRU {

It doesn't look like there were any references to these functions in the codebase so removing instead of porting is probably the right move, but it was a bit confusing at first glance. I think it might be better to do two PRs to keep this diff clean and have a place to provide context on what build_lru_index and restore_lru were initially for if it's at all relevant. I can quickly approve the removal one first and we can PR the rest on top of it.

---

nativelink-util/src/evicting_map.rs line 253 at r1 (raw file):

    /// Return the size of a `key`, if not found `None` is returned.
    pub async fn size_for_key(&self, key: &impl Borrow<K>) -> Option<usize> {

nit: you don't need the impl Borrow<K> or k.borrow() here

    pub async fn size_for_key(&self, key: &K) -> Option<usize> {
        let mut results = [None];
        self.sizes_for_keys([key], &mut results[..]).await;
        results[0]
    }

---

nativelink-util/src/evicting_map.rs line 276 at r1 (raw file):

                    // Since we are not inserting anythign we don't need to evict based
                    // on the size of the store.
                    let should_evict = self.should_evict(lru_len, entry, 0, u64::MAX);

I went down a rabbit hole refactoring this to be a bit clearer and ideally delegate each entry to another function but it's out of the scope of this PR and possibly not worthwhile. I do think this should be commented stating why getting the size for the keys implicitly evicts and that it uses the key rather than just peeking it.

[allada]

Reviewable status: 0 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @adam-singer and @zbirenbaum)

---

nativelink-util/src/evicting_map.rs line 194 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

It doesn't look like there were any references to these functions in the codebase so removing instead of porting is probably the right move, but it was a bit confusing at first glance. I think it might be better to do two PRs to keep this diff clean and have a place to provide context on what build_lru_index and restore_lru were initially for if it's at all relevant. I can quickly approve the removal one first and we can PR the rest on top of it.

Normally I would agree, but in this case it's a refactor anyway, and as long as we are not introducing features with modifications it should generally be considered ok.

If these functions were used somewhere in the code base it would not have been able to compile or a test would have failed (if it was referenced outside code base).

As a rule of thumb, deleting code is generally considered ok. I don't see how it makes code review more difficult, since it's just removal without modifications related to it.

---

nativelink-util/src/evicting_map.rs line 253 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

nit: you don't need the impl Borrow<K> or k.borrow() here

    pub async fn size_for_key(&self, key: &K) -> Option<usize> {
        let mut results = [None];
        self.sizes_for_keys([key], &mut results[..]).await;
        results[0]
    }

I'm using Borrow, so the caller doesn't need to actually need to be a K and instead just needs a way to reference into K. For example, we might implement:

impl Borrow<DigestInfo> for ActionInfo {
  fn borrow(a: &ActionInfo) -> &DigestInfo {
    a.digest
  }
}

It's following the same api that HashMap is following:
https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.get

[allada]

Reviewable status: 0 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @adam-singer and @zbirenbaum)

---

nativelink-util/src/evicting_map.rs line 276 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I went down a rabbit hole refactoring this to be a bit clearer and ideally delegate each entry to another function but it's out of the scope of this PR and possibly not worthwhile. I do think this should be commented stating why getting the size for the keys implicitly evicts and that it uses the key rather than just peeking it.

Done.

[zbirenbaum]

Reviewable status: 0 of 2 LGTMs obtained, and 1 discussions need to be resolved (waiting on @adam-singer)

---

nativelink-util/src/evicting_map.rs line 253 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I'm using Borrow, so the caller doesn't need to actually need to be a K and instead just needs a way to reference into K. For example, we might implement:

impl Borrow<DigestInfo> for ActionInfo {
  fn borrow(a: &ActionInfo) -> &DigestInfo {
    a.digest
  }
}

It's following the same api that HashMap is following:
https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.get

Got it, in that case it looks good

[zbirenbaum]

Reviewable status: 1 of 2 LGTMs obtained, and 1 discussions need to be resolved (waiting on @adam-singer)

[zbirenbaum]

Reviewable status: 1 of 2 LGTMs obtained, and 1 discussions need to be resolved (waiting on @adam-singer)

[allada]

-@adam-singer

Reviewable status: complete! 1 of 1 LGTMs obtained