fix: Fix data-race in lru-cache and 200x improvement in EraseEntriesWithGivenPrefix performance for deleting 10K entries out of 20K

[kislaykishore]

Description

• Fix a data race bug (fatal error: concurrent map iteration and map write) in lru where c.index was previously iterated without holding a lock while other operations might insert concurrently. While it doesn't cause issues in the applications since the operations to this module are already serialized, it's unintuitive and a code-smell.
• Refactored Erase and EraseEntriesWithGivenPrefix to use a common internal lock-free eraseInternal utility function.
• Ensured that EraseEntriesWithGivenPrefix will acquire the write lock once for the entire batch instead of taking it for every entry.
• Add benchmark tests for lru-cache erasure for a given prefix that show 18x performance improvement for erasing entries in bulk.

Perf

Before fix

goos: linux
goarch: amd64
pkg: github.com/googlecloudplatform/gcsfuse/v3/internal/cache/lru
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkEraseEntriesWithGivenPrefix-16                     1628            747389 ns/op
BenchmarkEraseEntriesWithGivenPrefix_Concurrent-16      fatal error: concurrent map iteration and map write

After fix

[----------] Finished with tests from CacheTest
goos: linux
goarch: amd64
pkg: github.com/googlecloudplatform/gcsfuse/v3/internal/cache/lru
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
BenchmarkEraseEntriesWithGivenPrefix-16                    30418             39771 ns/op
BenchmarkEraseEntriesWithGivenPrefix_Concurrent-16        242079              4817 ns/op

Link to the issue in case of a bug fix.

b/495217870

Testing details

1. Manual - NA
2. Unit tests - Added
3. Integration tests - NA

Any backward incompatible change? If so, please explain.

No

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical data race within the LRU cache implementation, specifically when iterating and modifying the cache's index concurrently. The changes introduce a more robust and performant approach to erasing entries, particularly for prefix-based deletions, by carefully managing lock acquisition and delegating core removal logic to an internal helper. This ensures the cache operates safely and efficiently under concurrent access patterns.

Highlights

• Data Race Fix: Resolved a data race bug (concurrent map iteration and write) in the LRU cache's EraseEntriesWithGivenPrefix method by ensuring proper locking during iteration and deletion.
• Refactoring Erase Operations: Refactored the Erase and EraseEntriesWithGivenPrefix methods to utilize a new internal, lock-free utility function eraseInternal for core deletion logic.
• Optimized Batch Deletion: Improved EraseEntriesWithGivenPrefix to acquire a read lock for key collection and then a single write lock for batch deletion, enhancing performance and concurrency safety.
• New Unit Tests: Added a new unit test case to specifically cover concurrent operations on EraseEntriesWithGivenPrefix, validating the fix and new behavior.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively resolves a data race in the EraseEntriesWithGivenPrefix function within the LRU cache. The fix refactors the deletion logic to first collect all keys for deletion under a read lock, and then perform a batch deletion under a single write lock. This not only fixes the concurrent map iteration and map write error but also improves performance by reducing lock contention. A new concurrent test has been added to validate the fix. The implementation is clean and correctly addresses the issue.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.71%. Comparing base (8d8d444) to head (b0da3d0).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4515      +/-   ##
==========================================
- Coverage   83.71%   83.71%   -0.01%     
==========================================
  Files         163      163              
  Lines       20078    20090      +12     
==========================================
+ Hits        16809    16818       +9     
- Misses       2646     2648       +2     
- Partials      623      624       +1     

Flag	Coverage Δ
unittests	83.71% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.