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

Make a single call to run_pending_tasks to evict as many entries as possible from the cache #417

Merged
merged 9 commits into from
Apr 16, 2024

Conversation

tatsuya6502
Copy link
Member

@tatsuya6502 tatsuya6502 commented Apr 14, 2024

Fixes #408.

Changes

This PR ensures that a single call to run_pending_tasks to evict as many entries as possible from the cache.

  • Disable the batch limits when the eviction listener is not set to the cache.
    • So single run_pending_tasks call should remove all the entries that can be evicted from the cache.
  • Add a hard-coded timeout duration for the maintenance task.
    • When the eviction listener is set, run_pending_tasks should stop (return) after the timeout duration is elapsed.
    • This is a safe-guard to prevent the maintenance task from running long time when the user provided a slow eviction listener.
    • In older versions, the batch size was used to limit the time spent on the maintenance task.
    • Note that run_pending_tasks checks the timeout only after processing a batch of entries, so run_pending_tasks can run longer than the timeout duration.
  • Reduce the size of the read op and write op channels.

Tests

  1. Add a unit test using a cache with no eviction listener. It verifies that a single call to run_pending_tasks to evict as many entries as possible.
  2. Add a unit test using a cache with a eviction listener. It verifies that timeout duration is working.

@tatsuya6502 tatsuya6502 added the enhancement New feature or request label Apr 14, 2024
@tatsuya6502 tatsuya6502 added this to the v0.12.7 milestone Apr 14, 2024
@tatsuya6502 tatsuya6502 changed the title Make run_pending_tasks to evict more entries from the cache at once Make a single call to run_pending_tasks to evict as many entries as possible from the cache Apr 14, 2024
@tatsuya6502 tatsuya6502 marked this pull request as ready for review April 14, 2024 11:23
@tatsuya6502 tatsuya6502 self-assigned this Apr 14, 2024
@tatsuya6502 tatsuya6502 marked this pull request as draft April 15, 2024 09:15
@tatsuya6502
Copy link
Member Author

tatsuya6502 commented Apr 16, 2024

A CI job for Kani verifier is failing with compile errors on proc-macro2 crate. It is not related to the changes in this pull request.

We will address the Kani and proc-macro2 issue separately in the following:

- Disable the eviction batch size when the eviction listener is not set to the cache.
  - So single `run_pending_tasks` call should remove all entries that can be evicted
    from the cache.
- Add a hard-coded maintenance task timeout duration.
  - When the eviction listener is set, `run_pending_tasks` should stop (return) after
    the maintenance task timeout is elapsed.
  - This is a safe-guard to prevent the maintenance task from running long time when
    the user wrote a slow eviction listener.
  - In older versions, the batch size was used to limit the time spent on the
    maintenance task.
  - Not that `run_pending_tasks` checks the timeout only after processing a batch
    of entries, so `run_pending_tasks` can run longer than the timeout duration.
- Reduce the size of the read op and write op channels.
Apply the same change of `future::Cache` to `sync` caches.
possible from the cache

Ensure the loop in the `do_run_pending_tasks` method is eventually stopped.
possible from the cache

Tweak some unit tests.
@tatsuya6502 tatsuya6502 marked this pull request as ready for review April 16, 2024 11:28
Copy link
Member Author

@tatsuya6502 tatsuya6502 left a comment

Choose a reason for hiding this comment

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

Merging.

@tatsuya6502 tatsuya6502 merged commit c56d646 into main Apr 16, 2024
39 checks passed
@tatsuya6502 tatsuya6502 deleted the evict-more-entries-at-once branch April 16, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inefficient size aware eviction when inserting new entries with larger size
1 participant