perf: Optimize dirInode prevDirListingTimeStamp by using atomic.Int64 instead of time.Time #4862
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the prevDirListingTimeStamp field in dirInode from time.Time to atomic.Int64 to ensure thread-safe operations, updating the associated cache invalidation logic and unit tests. The review feedback highlights critical issues where calling UnixNano() on an uninitialized or zero time.Time value will cause a panic in Go. Additionally, the feedback suggests handling potential clock skew defensively to prevent the cache from serving stale data indefinitely.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## radix-trie-cache #4862 +/- ##
===================================================
Coverage ? 83.73%
===================================================
Files ? 171
Lines ? 21117
Branches ? 0
===================================================
Hits ? 17683
Misses ? 2770
Partials ? 664
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4d3a321 to
4db3346
Compare
Description
This PR optimizes the
dirInodestruct by converting theprevDirListingTimeStampfield from atime.Timestruct into anatomic.Int64representing Unix nanoseconds. This reduces memory overhead per cached directory and eliminates a latent thread-safety data race during concurrent kernel cache TTL checks.Latent Data Race Fix (Concurrency Safety) : Previously,
ShouldInvalidateKernelListCachewas performing concurrent, lock-free reads on the 24-bytetime.Timestruct. Because a 24-byte struct cannot be read in a single CPU instruction, this exposed the cache invalidation path to memory tearing (reading a partially-updated timestamp) during highly parallel FUSE directory lookups. By migrating toatomic.Int64.Load()and.Store(), we guarantee 100% thread-safe, lock-free reads at the hardware level without introducing any Mutex overhead.Memory Optimization : A time.Time struct consumes 24 bytes in Go. An
atomic.Int64consumes 8 bytes. Because GCSFuse heavily caches directories, shrinking this auxiliary field removes exactly 16 bytes from everydirInodein memory (reducing its size from 344B to 328B). This results in a ~1.6 MB heap RAM savings per 100,000 cached directories.All existing directory and inode tests pass successfully.
Link to the issue in case of a bug fix.
Testing details
Test_ShouldInvalidateKernelListCache_RaceConditionto check for Race condition that is fixed by usingatomic.Int64. To use the same test for the version of dirInode withtime.Timechange thed.prevDirListingTimeStamp.Store(d.cacheClock.Now().UnixNano())tod.prevDirListingTimeStamp = d.cacheClock.Now()Any backward incompatible change? If so, please explain.