perf(stat-cache memory): storing expiry nanoseconds (int64) instead of time.Time#4825
perf(stat-cache memory): storing expiry nanoseconds (int64) instead of time.Time#4825raj-prince wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
remind-reviewers
Summary of ChangesHello, I'm Gemini Code Assist1! 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 optimizes the memory usage of the stat cache by storing expiration times as int64 nanosecond timestamps rather than full time.Time objects. This change reduces the size of each cache entry and improves performance for insertion and lookup operations, as confirmed by the included benchmark results. Highlights
Using Gemini Code AssistThe 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
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the metadata stat cache by changing the expiration field in the cache entry from time.Time to an int64 nanosecond timestamp, which reduces memory overhead and improves comparison performance. It also introduces benchmark tests for the cache operations. The feedback suggests optimizing the lookup benchmark by pre-allocating the keys to avoid string allocation overhead inside the benchmark loop.
| now := time.Now() | ||
|
|
||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| for i := 0; i < b.N; i++ { | ||
| name := fmt.Sprintf("file-%d", i%10000) | ||
| sc.LookUp(name, now) | ||
| } |
There was a problem hiding this comment.
In BenchmarkStatCache_LookUp, calling fmt.Sprintf inside the benchmark loop introduces unnecessary allocations. Per repository guidelines, use strconv.AppendInt with a pre-allocated byte slice to build strings efficiently and minimize memory allocations.
names := make([]string, 10000)
buf := make([]byte, 0, 20)
for i := 0; i < 10000; i++ {
buf = append(buf[:0], "file-"...)
buf = strconv.AppendInt(buf, int64(i), 10)
names[i] = string(buf)
}
now := time.Now()
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
sc.LookUp(names[i%10000], now)
}References
- In performance-sensitive Go code, avoid string concatenation with the + operator. Instead, build the string using a pre-allocated byte slice and append functions (e.g., strconv.AppendInt) to minimize memory allocations.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4825 +/- ##
==========================================
- Coverage 83.68% 83.65% -0.04%
==========================================
Files 168 168
Lines 20864 20864
==========================================
- Hits 17461 17454 -7
- Misses 2753 2758 +5
- Partials 650 652 +2
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:
|
kislaykishore
left a comment
kislaykishore
left a comment
There was a problem hiding this comment.
Gave some comments for the first method. Please incorporate them into other methods as well.
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
|
|
||
| for i := 0; i < b.N; i++ { |
| b.ReportAllocs() | ||
|
|
||
| for i := 0; i < b.N; i++ { | ||
| name := fmt.Sprintf("file-%d", i) |
There was a problem hiding this comment.
Using sprintf affects the benchmarking results. Prefer using strings.Builder with strconv for formatting integers.
| expiration := time.Now().Add(time.Hour) | ||
|
|
||
| b.ResetTimer() | ||
| b.ReportAllocs() |
There was a problem hiding this comment.
Why is this needed? You could achieve the same using benchmem
| sc := metadata.NewStatCacheBucketView(lc, "") | ||
| expiration := time.Now().Add(time.Hour) | ||
|
|
||
| b.ResetTimer() |
There was a problem hiding this comment.
If you use b.loop, this will not be needed.
| expiration time.Time | ||
| m *gcs.MinObject | ||
| f *gcs.Folder | ||
| expirationNS int64 |
There was a problem hiding this comment.
Why do we need nanoseconds precision here?
There was a problem hiding this comment.
Our ttls are in seconds. Wdyt?
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
5 similar comments
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Hi @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
|
Closing in favor of - #4857 |
3 participants
Description
Link to the issue in case of a bug fix.
Testing details
Any backward incompatible change? If so, please explain.