perf: optimize TTL expiration with runtime.nanotime() and consolidated type assertions#21
perf: optimize TTL expiration with runtime.nanotime() and consolidated type assertions#21analytically wants to merge 2 commits intogo-pkgz:masterfrom
Conversation
…d type assertions Replace time.Time with int64 nanotime values for expiration tracking, simplifying TTL checks and reducing memory footprint. 19-30% faster. Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
|
@paskal could you review? |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes cache performance by replacing time.Time with int64 nanotime values for TTL expiration tracking, achieving 19-30% performance improvements across all operations.
Key changes:
- Use runtime.nanotime() via //go:linkname for fastest monotonic time access
- Store expiration as int64 nanoseconds instead of time.Time structures
- Consolidate type assertions to reduce redundant operations
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| v3/cache.go | Core optimization replacing time.Time with int64 nanotime, consolidating type assertions, and updating all expiration logic |
| v3/options.go | Convert TTL parameter from time.Duration to int64 nanoseconds |
| v3/go.mod | Update Go version to 1.25 and remove unused dependency |
| v3/cache_test.go | Remove unused import and test function for removed dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
There was a problem hiding this comment.
Thanks for the effort, but I don't think we should merge this PR. Here are the concerns:
-
//go:linknameto internal runtime function —runtime.nanotime()is an unexported internal function. Using//go:linknameto access it is fragile and explicitly discouraged by the Go team, who have been actively locking down linkname access. This could break on any Go version upgrade without warning. -
Breaking API changes — The PR removes
GetExpiration(key K) (time.Time, bool)from the public interface and dropssimplelru.LRUCacheinterface compliance (along with thehashicorp/golang-lru/v2dependency). Users relying on these would break. -
Incompatible with
testing/synctest— Go 1.24 introducedtesting/synctestwhich provides a fake clock that interceptstime.Now()automatically. This allows deterministic testing of TTL behaviour without any library changes. Switching toruntime.nanotime()would make the cache immune to this fake clock, preventing users (and our own tests) from using this facility. See issue #24 for context. -
Performance vs maintenance trade-off — While the benchmark improvements (20-30%) are real, the absolute times (~175-250ns per operation) are already very fast. For a caching library, the overhead of
time.Now()vsruntime.nanotime()is typically negligible compared to the actual workload being cached. The performance gain doesn't justify the maintenance risk and API breakage.
We're going to rewrite our tests to use testing/synctest instead, which demonstrates that time.Now() is the right approach — it works with Go's testing infrastructure out of the box.
Replace time.Time with int64 nanotime values for expiration tracking, simplifying TTL checks and reducing memory footprint. 19-30% faster.
Performance improvements (benchstat with n=3 on Apple M1):
Changes:
Benefits:
Note: runtime.nanotime() is an internal Go API that may change in future
versions. Trade-off accepted for significant performance and correctness benefits.