ExistenceCacheStore now only evicts based on insert #1203
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 9 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable (waiting on @adam-singer)
allada
force-pushed
the
fix-existence-cache-to-only-evict-based-on-insert-time
branch
3 times, most recently
from
86ba193
to
1ae1e9c
Compare
To protect against a case where the parent layer store has items hot in cache and those items are frequently touched, we would never actually ask the underlying store for the data. This caused a problem because the underlying store might evict our item, then our parent ExistenceCacheStore would never let go of the item causing the node to be required to shutdown to clear it up. This was a vary rare event, because most items have `.get()` eventually called on the digest, which refreshes the underlying store object. closes TraceMachina#1199
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable
allada
force-pushed
the
fix-existence-cache-to-only-evict-based-on-insert-time
branch
from
1ae1e9c
to
130d10b
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Remote / large-ubuntu-22.04
allada
deleted the
fix-existence-cache-to-only-evict-based-on-insert-time
branch
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 1 LGTMs obtained, and all files reviewed
To protect against a case where the parent layer store has items hot in cache and those items are frequently touched, we would never actually ask the underlying store for the data. This caused a problem because the underlying store might evict our item, then our parent ExistenceCacheStore would never let go of the item causing the node to be required to shutdown to clear it up.
This was a vary rare event, because most items have
.get()eventually called on the digest, which refreshes the underlying store object.closes #1199
This change is