Dynamically resizes cache depending on the amount of RAM available (issue #167) #213
Conversation
Codecov Report
@@ Coverage Diff @@
## main #213 +/- ##
==========================================
- Coverage 55.65% 54.35% -1.30%
==========================================
Files 82 83 +1
Lines 3249 3467 +218
==========================================
+ Hits 1808 1884 +76
- Misses 1259 1392 +133
- Partials 182 191 +9
Continue to review full report at Codecov.
|
petethepig
changed the title
pkg/storage/cache/cache.go
Outdated
| <-cache.cleanupDone | ||
| } | ||
|
|
||
| func (cache *Cache) Evit(percent float64) { |
There was a problem hiding this comment.
Is this a typo? I think you meant Evict
pkg/storage/storage.go
Outdated
| @@ -163,6 +164,42 @@ func New(cfg *config.Config) (*Storage, error) { | |||
| return tree.New() | |||
| } | |||
|
|
|||
| // load the total memory of the server | |||
| vm, err := mem.VirtualMemory() | |||
There was a problem hiding this comment.
Can you please add tests for this whole PR?
For tests we could add a way to set this limit arbitrarily, to something like 1 MB, and then make sure the system still works.
There was a problem hiding this comment.
already add the test for this PR
pkg/storage/storage.go
Outdated
| // start a timer for checking if the memory used by application | ||
| // is more than 25% of the total memory, if so, trigger eviction | ||
| // with 10% to every cache | ||
| go func() { |
There was a problem hiding this comment.
Perhaps it would be better to have this as a separate function.
pkg/storage/storage.go
Outdated
| // is more than 25% of the total memory, if so, trigger eviction | ||
| // with 10% to every cache | ||
| go func() { | ||
| ticker := time.NewTimer(interval * time.Second) |
There was a problem hiding this comment.
I think a Ticker is more appropriate here.
There was a problem hiding this comment.
- The
Timertype represents a single event - A
Tickerholds a channel that delivers “ticks” of a clock at intervals
Timers are for when you want to do something once in the future - tickers are for when you want to do something repeatedly at regular intervals.
There was a problem hiding this comment.
yes, but i reset the timer in the loop
// reset the timer
ticker.Reset(interval * time.Second)
There was a problem hiding this comment.
For sure that works. However, I think it would be better to use more idiomatic approach with Ticker :)
There was a problem hiding this comment.
@kolesnikovae I talked with @alonlong about it. I think the idea is that if this procedure takes longer than the timer we want to wait for another X seconds (10 right now I think) before flushing.
So I think it's fine. Although, I wonder if we should just add a time.Sleep there. What do you both think about that? I think that would make code more concise, but maybe there're some downsides I'm not thinking of.
There was a problem hiding this comment.
Ticker also has Reset method for that.
Sorry guys if led you astray. Please don't get me wrong, I was just trying to say that Timer and Ticker have different semantics, and for this particular case the last one is more appropriate IMO. Taking into account the mismatch of the variable name and the type it's a bit confusing: is it supposed to be a timer and the variable name should be changed, or is it a ticker indeed and the type should be changed? Both options are okay.
There was a problem hiding this comment.
@kolesnikovae Got it, I think we can change variable name to timer
pkg/storage/storage.go
Outdated
| defer ticker.Stop() | ||
|
|
||
| for range ticker.C { | ||
| if s.closing { |
There was a problem hiding this comment.
I guess this will cause races.
There was a problem hiding this comment.
can u help to describe the races?
There was a problem hiding this comment.
There is a race condition on access to closing field, you should acquire closingMutex and release it only after Evict calls. See Close: basically, I think that we could replace the mutex with an RWMutex.
There was a problem hiding this comment.
for range ticker.C {
if s.closing {
return
}
There was a problem hiding this comment.
there is no closingMutex here
There was a problem hiding this comment.
actually, i think for the field closing, the mutex is not necessary. the code is weird for using the mutex for closing
There was a problem hiding this comment.
I'm afraid, without a mutex, there is no other way to avoid the race (without changing too many things): eventually the call will conflict with Close.
closingMutex is a member of Storage:
https://github.com/pyroscope-io/pyroscope/blob/f45b14e491357af26f8a32e989ef573d1b86e8d3/pkg/storage/storage.go#L38-L40
There was a problem hiding this comment.
it's a bool variable, i think it's not a problem
There was a problem hiding this comment.
Regardless of the type, when the same address accessed for read-write concurrently, a race condition occurs. The funny part is that it would be safer to not check closing field at all in this particular function, because Evict and Flush (in Close) methods can be called in arbitrary order with no issues :)
…e' into feature/resize_cache
…fter server closes
Raise limits of buffer sizes
Fix the issue: Dynamically resize cache depending on the amount of RAM available #167