Skip to content

Conversation

@analytically
Copy link
Contributor

Improve performance in frequently used cache methods:

  1. Values method:

    • Move time.Now() outside of lock
    • Simplify iteration
  2. DeleteExpired method:

    • Replace key slice allocation with direct list traversal
    • Cache time.Now() call for efficiency, move out of lock
    • Avoid redundant map lookups
    • Properly handle list traversal during element removal
  3. Add method:

    • Cache time.Now() and move out of lock

All optimizations preserve API compatibility while reducing allocations and CPU work.

Improve performance in frequently used cache methods:

1. Values method:
   - Move time.Now() outside of lock
   - Simplify iteration

2. DeleteExpired method:
   - Replace key slice allocation with direct list traversal
   - Cache time.Now() call for efficiency, move out of lock
   - Avoid redundant map lookups
   - Properly handle list traversal during element removal

3. Add method:
   - Cache time.Now() and move out of lock

All optimizations preserve API compatibility while reducing allocations and CPU work.

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
@analytically analytically requested a review from umputun as a code owner March 28, 2025 11:13
@analytically
Copy link
Contributor Author

analytically commented Mar 28, 2025

goos: darwin
goarch: arm64
pkg: github.com/go-pkgz/expirable-cache/v3
cpu: Apple M1 Pro
                       │  master.txt  │              perf.txt               │
                       │    sec/op    │    sec/op     vs base               │
LRU_Rand_NoExpire-10     179.7n ± ∞ ¹   169.3n ± ∞ ¹   -5.79% (p=0.016 n=5)
LRU_Freq_NoExpire-10     173.0n ± ∞ ¹   167.5n ± ∞ ¹        ~ (p=0.222 n=5)
LRU_Rand_WithExpire-10   203.2n ± ∞ ¹   176.2n ± ∞ ¹  -13.29% (p=0.008 n=5)
LRU_Freq_WithExpire-10   197.2n ± ∞ ¹   175.2n ± ∞ ¹  -11.16% (p=0.008 n=5)
geomean                  187.9n         172.0n         -8.44%
¹ need >= 6 samples for confidence interval at level 0.95

@analytically
Copy link
Contributor Author

analytically commented Mar 28, 2025

Moving time.Now() before acquiring the lock reduces the critical section duration, which can significantly improve concurrency in high-contention scenarios. The exact timestamp precision is not critical (a few microseconds earlier doesn't matter), and potentially you could say that the method called time is the mark. And time.Now() can be relatively expensive (vDSO/syscall), so performing it outside the lock helps reduce contention.

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
@paskal paskal self-assigned this Mar 30, 2025
@paskal paskal requested review from Copilot and paskal March 30, 2025 19:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request optimizes core cache operations for performance improvements in Go 1.24 by reducing lock duration and avoiding redundant operations.

  • Time-critical methods (Set/addWithTTL, Values, and DeleteExpired) now cache time.Now() outside of locks.
  • The deletion of expired elements is streamlined by directly traversing the eviction list.
  • Legacy helper removeOldestIfExpired is removed in favor of inline removal logic.
Files not reviewed (1)
  • v3/go.mod: Language not supported
Comments suppressed due to low confidence (1)

v3/cache.go:207

  • [nitpick] Consider using a more conventional comparison such as now.Before(expiration) for clarity.
if !now.After(ent.Value.(*cacheItem[K, V]).expiresAt) {

paskal added 3 commits March 31, 2025 15:48
- Move ttl check and time.Now() outside lock to reduce lock contention
- Optimize DeleteExpired with direct list traversal
- Remove unused removeOldestIfExpired method
- Use same inline expiration check
- Update benchmark results in main README with latest performance metrics
- Update benchmarks README with current numbers
- Adjust performance improvement percentages to reflect latest changes
@coveralls
Copy link

Pull Request Test Coverage Report for Build 14173478019

Details

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 13825013328: 0.0%
Covered Lines: 168
Relevant Lines: 168

💛 - Coveralls

@paskal
Copy link
Collaborator

paskal commented Mar 31, 2025

I've added the same optimisations to v1 and v2, re-run the benchmarks and updated the results in readme, and also reverted the upgrade of go version to 1.24 as it's up to the caller and doesn't make sense to make a requirement from the library from my perspective. I'll release the new version once it's merged.

@umputun, could you please take a look?

@umputun umputun requested a review from Copilot April 15, 2025 01:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

@umputun umputun merged commit c006c00 into go-pkgz:master Apr 15, 2025
1 check passed
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.

4 participants