Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lfu support for debug object command. #479

Open
wants to merge 3 commits into
base: unstable
Choose a base branch
from

Conversation

DarrenJiang13
Copy link

@DarrenJiang13 DarrenJiang13 commented May 9, 2024

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
Copy link
Author

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

Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 69.84%. Comparing base (fdd023f) to head (a311b35).
Report is 7 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #479      +/-   ##
============================================
+ Coverage     69.81%   69.84%   +0.03%     
============================================
  Files           109      109              
  Lines         61792    61801       +9     
============================================
+ Hits          43139    43167      +28     
+ Misses        18653    18634      -19     
Files Coverage Δ
src/debug.c 53.54% <77.77%> (+0.07%) ⬆️

... and 16 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
Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

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
Copy link
Author

DarrenJiang13 commented May 10, 2024

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
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.

Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
Signed-off-by: jiangyujie.jyj <yjjiang1996@163.com>
@DarrenJiang13
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants