Skip to content

Conversation

@MarcusSorealheis
Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis commented Oct 1, 2025

Description

Customers have reported problems with to prevent eager evictions and cache thrash. The grace period optionally prevents recently added cache entries from eviction.

Fixes #1943

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@chrisstaite-menlo
Copy link
Collaborator

My main concern with this is that nativelink filesystem store does really badly when it reaches an out of disk space. This isn't an issue with the existing implementation, but with this there's a possibility that it can happen. I think if we're going to introduce this we also need to introduce a hard upper limit which starts evicting stuff that's in the grace period too.

@chrisstaite-menlo
Copy link
Collaborator

nativelink-util/src/evicting_map.rs line 291 at r9 (raw file):

        // Check if item is within grace period
        let within_grace_period = if self.eviction_grace_period_seconds > 0 {
            let current_time_seconds = self.anchor_time.elapsed().as_secs() as i32;

as i32 can panic, it's better not to use it. I thought we had a clippy for that?

Copy link
Collaborator

@chrisstaite-menlo chrisstaite-menlo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and 0 of 5 files reviewed, and 7 discussions need to be resolved


nativelink-util/src/evicting_map.rs line 321 at r9 (raw file):

        };

        let (should_evict_initial, _within_grace) = self.should_evict_with_grace_check(

Previously the work of calling should_evict was gated behind self.max_bytes and self.evict_bytes. Now this work is done upfront unnecessarily.


nativelink-util/src/evicting_map.rs line 348 at r9 (raw file):

                    // Track that grace period blocked an eviction
                    state.grace_period_blocks.inc();
                    info!(

This could be a very noisy log, it should probably be debug level.

@MarcusSorealheis
Copy link
Collaborator Author

My main concern with this is that nativelink filesystem store does really badly when it reaches an out of disk space. This isn't an issue with the existing implementation, but with this there's a possibility that it can happen. I think if we're going to introduce this we also need to introduce a hard upper limit which starts evicting stuff that's in the grace period too.

This is a very good point. I need to do some more thinking here.

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.

3 participants