Skip to content

Add lfu support for DEBUG OBJECT command, added lfu_freq and lfu_access_time_minutes fields#479

Merged
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
DarrenJiang13:add-lfu-when-debug-object
Aug 16, 2024
Merged

Add lfu support for DEBUG OBJECT command, added lfu_freq and lfu_access_time_minutes fields#479
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
DarrenJiang13:add-lfu-when-debug-object

Conversation

@DarrenJiang13

@DarrenJiang13 DarrenJiang13 commented May 9, 2024

Copy link
Copy Markdown
Contributor

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

Added two new fields lfu_freq and lfu_access_time_minutes.

@DarrenJiang13

Copy link
Copy Markdown
Contributor Author

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

@codecov

codecov Bot commented May 9, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️

... and 13 files with indirect coverage changes

@DarrenJiang13 DarrenJiang13 force-pushed the add-lfu-when-debug-object branch 2 times, most recently from 9eac8da to f713c9e Compare May 9, 2024 17:15
@madolson madolson requested review from chenyang8094 and soloestoy May 9, 2024 21:59
@DarrenJiang13 DarrenJiang13 force-pushed the add-lfu-when-debug-object branch from f713c9e to 311fac2 Compare May 10, 2024 01:50

@enjoy-binbin enjoy-binbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DarrenJiang13

DarrenJiang13 commented May 10, 2024

Copy link
Copy Markdown
Contributor Author

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.

@enjoy-binbin

Copy link
Copy Markdown
Member

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.

@DarrenJiang13

Copy link
Copy Markdown
Contributor Author

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.

@enjoy-binbin

Copy link
Copy Markdown
Member

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

@DarrenJiang13 DarrenJiang13 force-pushed the add-lfu-when-debug-object branch from a311b35 to 0ae843b Compare August 16, 2024 03:56
Comment thread src/debug.c Outdated
@DarrenJiang13 DarrenJiang13 force-pushed the add-lfu-when-debug-object branch from 0ae843b to 52ee903 Compare August 16, 2024 07:54
@enjoy-binbin

Copy link
Copy Markdown
Member

please sign the DCO

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
@DarrenJiang13 DarrenJiang13 force-pushed the add-lfu-when-debug-object branch from 52ee903 to 02d2b60 Compare August 16, 2024 09:38
@enjoy-binbin enjoy-binbin changed the title Add lfu support for debug object command. Add lfu support for DEBUG OBJECT command, added lfu_freq and lfu_access_time_minutes fields Aug 16, 2024
@enjoy-binbin enjoy-binbin merged commit adf53c2 into valkey-io:unstable Aug 16, 2024
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 21, 2024
…ss_time_minutes fields (valkey-io#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: mwish <maplewish117@gmail.com>
mapleFU pushed a commit to mapleFU/valkey that referenced this pull request Aug 22, 2024
…ss_time_minutes fields (valkey-io#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: mwish <maplewish117@gmail.com>
madolson pushed a commit that referenced this pull request Sep 2, 2024
…ss_time_minutes fields (#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
madolson pushed a commit that referenced this pull request Sep 3, 2024
…ss_time_minutes fields (#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Sep 4, 2024
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…ss_time_minutes fields (valkey-io#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Ping Xie <pingxie@outlook.com>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…ss_time_minutes fields (valkey-io#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Ping Xie <pingxie@google.com>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…ss_time_minutes fields (valkey-io#479)

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

Added two new fields lfu_freq and lfu_access_time_minutes.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes This issue should get a line item in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants