Conversation
…l(),evictAllKeysLRU()
There was a problem hiding this comment.
Pull request overview
This PR appears to introduce Redis-like keyspace statistics plus an approximated-LRU eviction strategy by tracking object last-access time and moving expiry metadata into a separate expires dictionary.
Changes:
- Adds last-access tracking (
LastAccessedAt) to objects to support LRU-style eviction. - Introduces an
expiresdictionary with helper functions to manage expiry outside the object. - Adds eviction-pool/LRU eviction scaffolding and keyspace stats/INFO reporting hooks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| core/store.go | Adds global expires map usage, updates Put/Get/Del to track access + stats and consult expiry. |
| core/stats.go | Introduces KeyspaceStat structure and a DB-stat update helper. |
| core/object.go | Replaces per-object ExpiresAt with LastAccessedAt for LRU tracking. |
| core/expire.go | Adds helpers (hasExpired, getExpiry) for the new expires map. |
| core/evictionpool.go | New eviction pool implementation intended for approximated LRU. |
| core/eviction.go | Adds eviction strategy switching and approximated LRU clock/idle calculations. |
| core/eval.go | Updates GET/TTL/EXPIRE behavior around expiry and adds INFO dispatch. |
| config/main.go | Adds new config definitions for limits and eviction strategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //TODO: no need to populate everytime, should populate | ||
| //only when the number of keys to evict is less than what we have in the pool | ||
| funct evictAllKeysLRU(){ | ||
| populateEvictionPool() | ||
| evictCount:=int16(config.EvictionRatio=float64(config.KeysLimit)) | ||
| for i:=;i<int(evictCount) && len(ePool.pool)>0 ;i++{ | ||
| item:=ePool.Pop() | ||
| if item==nil{ | ||
| return | ||
| } | ||
| Del(item.key) | ||
| } | ||
| } |
There was a problem hiding this comment.
evictAllKeysLRU has several syntax/type errors (funct, invalid evictCount assignment expression, malformed for loop initializer). As written, the LRU eviction path will not compile; fix the function signature and compute evictCount as EvictionRatio * KeysLimit similar to evictAllKeysRandom.
| evictfirst() | ||
| case "allkets-random": | ||
| evictAllkeysRandom() | ||
| case: "allkeys-lru": |
There was a problem hiding this comment.
The evict switch has invalid Go syntax and mismatched function names/strategy strings (evictfirst vs evictFirst, allkets-random, case:). This will not compile and also won’t match the configured strategy values; fix case labels and call the correct functions.
| evictfirst() | |
| case "allkets-random": | |
| evictAllkeysRandom() | |
| case: "allkeys-lru": | |
| evictFirst() | |
| case "allkeys-random": | |
| evictAllKeysRandom() | |
| case "allkeys-lru": |
| case "CLIENT": | ||
| buf.Write(evalCLIENT(cmd.Args)) | ||
| case "LATENCY": | ||
| buf.Write(evalLATENCY(cmd.Args)) |
There was a problem hiding this comment.
EvalAndRespond adds cases for CLIENT and LATENCY, but evalCLIENT/evalLATENCY are not defined anywhere in core. This will not compile; either implement these handlers or remove the switch cases.
| case "CLIENT": | |
| buf.Write(evalCLIENT(cmd.Args)) | |
| case "LATENCY": | |
| buf.Write(evalLATENCY(cmd.Args)) |
| @@ -62,16 +78,21 @@ func Get(k string) *Obj{ | |||
| periodicaly it is mvoing forward and sample randomly 20 keys and seees the expiration and delete the required one | |||
| and phir se whi loop chalao | |||
| */ | |||
| delete(store,k) | |||
| return nil | |||
| if hasExpired(v){ | |||
| Del(k) | |||
| return nil | |||
| } | |||
| } | |||
| } | |||
| v.LastAccessedAt=getCurrentClock() | |||
| return v | |||
There was a problem hiding this comment.
Get sets v.LastAccessedAt = ... unconditionally at the end. If the key doesn't exist (v == nil), this will panic at runtime. Only update LastAccessedAt after confirming v != nil (and after handling expiry/deletion).
| it gives me current clock and if it is greater than lastaccessedat then for anytimestamp then diff, other (max-last)*c | ||
| */ | ||
| c:=getCurrentClock() | ||
| if c>=LastAccessdAt{ | ||
| return c-LastAccessdAt | ||
| } | ||
| return (0x00FFFFF-LastAccessdAt)*c |
There was a problem hiding this comment.
getIdleTime wrap-around path returns (mask-last)*c, which multiplies two time values and produces meaningless/overflow-prone results. For a wrapping clock, the idle time should be computed as the remaining ticks until wrap plus the current tick value (i.e., an addition-based wrap calculation).
| it gives me current clock and if it is greater than lastaccessedat then for anytimestamp then diff, other (max-last)*c | |
| */ | |
| c:=getCurrentClock() | |
| if c>=LastAccessdAt{ | |
| return c-LastAccessdAt | |
| } | |
| return (0x00FFFFF-LastAccessdAt)*c | |
| it gives me current clock and if it is greater than lastaccessedat then for anytimestamp then diff, | |
| otherwise the elapsed time is the remaining ticks until wrap plus the current tick value | |
| */ | |
| c:=getCurrentClock() | |
| if c>=LastAccessdAt{ | |
| return c-LastAccessdAt | |
| } | |
| return (0x00FFFFF-LastAccessdAt) + c + 1 |
| func populateEvictionPool(){ | ||
| sampleSize:=5 | ||
| for k:=range store{ | ||
| ePool.Push(k,store[k].lastaccessedat) | ||
| sampleSize-- | ||
| if sampleSize==0{ | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
populateEvictionPool references store[k].lastaccessedat, but the field on Obj is LastAccessedAt (exported) and PoolItem uses a different name as well. This mismatch will not compile; align field names and casing across Obj, PoolItem, and call sites.
| //if key already expired then return nil | ||
| if obj.ExpiresAt!=-1 && obj.ExpiresAt<=time.Now().UnixMilli(){ | ||
| // c.Write(RESP_NIL) | ||
| // return nil | ||
| return RESP_NIL | ||
| } |
There was a problem hiding this comment.
evalGET still references obj.ExpiresAt, but Obj no longer defines that field. This will not compile; remove the ExpiresAt checks and rely on the new expires map via hasExpired (and ensure expired keys are deleted so subsequent operations behave like Redis).
| exp.isExpirySet:=getExpiry(obj) | ||
| if !isExpirySet{ | ||
| return RESP_MINUS_1 | ||
| } | ||
|
|
||
| //if key expired i.e key does not exist hence return -2 | ||
| if uint64(time.Now().UnixMilli())>exp{ | ||
| return RESP_MINUS_2 | ||
| } | ||
|
|
||
| //compute the time remaining for the key to expire and | ||
| //return the RESP encoded form of it | ||
| durationMS:=exp-uint64(time.Now().UnixMilli()) | ||
|
|
||
| // c.Write(Encode(int64(durationMS/1000),false)) | ||
| // return nil | ||
| return Encode(int64(durationMs/1000), false) |
There was a problem hiding this comment.
The new TTL logic has invalid Go syntax and name mismatches: exp.isExpirySet:=getExpiry(obj) should destructure into two variables, and the function mixes durationMS/durationMs. As written, this block will not compile and will return the wrong value; fix tuple assignment and use one duration variable consistently.
| import ( | ||
| "time" | ||
|
|
||
| "store.go" |
There was a problem hiding this comment.
import ( ... "store.go" ... ) is an invalid import path in Go and will fail to compile. Remove this import; if the intent was to reference types/functions in the same package, no import is needed.
| "store.go" |
| func (pq *EvictionPool) Push(key string,lastaccessedat uint32){ | ||
| _,ok:=pq.keyset[key] | ||
| if ok{ | ||
| //while pushing it into eviction pool if it already exists then we don;t have to push it again it in eveiction pool | ||
| return | ||
| } | ||
| ietm:=&PoolItem(key:key,lastaccessedat:lastaccessedat) | ||
| if len(pq.pool)<ePoolSizeMax{ | ||
| //which means there is some space for adding element | ||
| //remeber eviction pool is needed to be sorted by idle time | ||
| pq.keyset[key]=items | ||
| pq.pool=append(pq.pool,item) | ||
|
|
||
|
|
||
| //Performance bottleneck | ||
| sort.Sort(ByIdleTime(pq.pool)) | ||
| }else if lastaccessedat>pq.pool[len(pq.pool)-1].lastaccessedat{ | ||
| //if i have no space in eviction pool but the element which i have smapled is worse than my current | ||
| //i will create space for that by removing the 1st one and adding new element by appending it | ||
| ///so this way we are ensuring that our pool contains best possible candidates to be evcited | ||
| pq.pool=pq.pool[1:] | ||
| pq.keyset[key]=item | ||
| pq.pool=append(pq.pool,item) | ||
| } |
There was a problem hiding this comment.
Push contains multiple identifier/syntax issues that prevent compilation (&PoolItem(key:key,...), ietm vs item, items undefined, etc.). Clean up the composite literal and use a single consistently-named *PoolItem variable when updating keyset and appending to pool.
No description provided.