valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Add lfu support for debug object command.

Open DarrenJiang13 opened this issue 1 year ago • 5 comments

Problem

For debug object command, we use val->lru but ignore the lfu mode.
So in lfu mode, debug object would return meaningless lru descriptions.

Solution:

In lfu mode:

127.0.0.1:6379> debug object a
Value at:0x600000afc060 refcount:1 encoding:embstr serializedlength:2 lfu_freq:7 lfu_access_time:13716

In other mode:

127.0.0.1:6379> debug object a
Value at:0x600000ac6bc0 refcount:1 encoding:embstr serializedlength:2 lru:3969189 lru_seconds_idle:2

DarrenJiang13 avatar May 09 '24 09:05 DarrenJiang13

I checked other place using obj->lru, it seems like to be the only part missing lfu condition.

DarrenJiang13 avatar May 09 '24 09:05 DarrenJiang13

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.34%. Comparing base (fc9f291) to head (02d2b60). Report is 58 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #479      +/-   ##
============================================
+ Coverage     70.22%   70.34%   +0.12%     
============================================
  Files           112      112              
  Lines         61497    61498       +1     
============================================
+ Hits          43185    43261      +76     
+ Misses        18312    18237      -75     
Files with missing lines Coverage Δ
src/debug.c 54.32% <100.00%> (+0.04%) :arrow_up:

... and 13 files with indirect coverage changes

codecov[bot] avatar May 09 '24 09:05 codecov[bot]

can we just add more fields after it? like:

127.0.0.1:6379> debug object a
Value at:0x600000ac6bc0 refcount:1 encoding:embstr serializedlength:2 lru:3969189 lru_seconds_idle:2 lfu_freq:7 lfu_access_time:13716

It is mainly used in DEBUG scenarios, so I think some different fields will not affect the use

hi @enjoy-binbin

struct serverObject {
    unsigned type:4;
    unsigned encoding:4;
    unsigned lru:LRU_BITS; /* LRU time (relative to global lru_clock) or
                            * LFU data (least significant 8 bits frequency
                            * and most significant 16 bits access time). */
    int refcount;
    void *ptr;
};

As we use this one 24-bit field lru for either LRU or LFU data, we could not get both lfu and lru info. For example, if the eviction policy is set to be 'lfu' mode, then lru field would be meaningless.

DarrenJiang13 avatar May 10 '24 08:05 DarrenJiang13

we can just set the value to -1 if you think the value is meaningless.

since it is a debug command, I don't care about its meaningless values ​​in other fields in different modes. The caller knows which fields it should look at. The current diff is touching a lot of code and i don't like it very much.

enjoy-binbin avatar May 10 '24 08:05 enjoy-binbin

we can just set the value to -1 if you think the value is meaningless.

since it is a debug command, I don't care about its meaningless values ​​in other fields in different modes. The caller knows which fields it should look at. The current diff is touching a lot of code and i don't like it very much.

Yes, that makes sense. Just merge two fields into one reply.

DarrenJiang13 avatar May 10 '24 17:05 DarrenJiang13

@DarrenJiang13 Hi, sorry for the late repsonse, would you be able to refresh this PR?

enjoy-binbin avatar Aug 16 '24 02:08 enjoy-binbin

please sign the DCO

enjoy-binbin avatar Aug 16 '24 08:08 enjoy-binbin