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

create a key succeed by INCR command,but it's LFU is not initial value 5. #4926

Open
FNST-zhoush opened this issue May 17, 2018 · 9 comments

Comments

@FNST-zhoush
Copy link

127.0.0.1:4001> incr key
(integer) 1
127.0.0.1:4001> object freq key
(integer) 0
127.0.0.1:4001> incr key
(integer) 2
127.0.0.1:4001> object freq key
(integer) 1
127.0.0.1:4001> set key2 1
OK
127.0.0.1:4001> object freq key2
(integer) 5

@youjiali1995
Copy link
Contributor

Redis creates shared objects including integer 0-9999.

@soloestoy
Copy link
Collaborator

Redis creates shared objects including integer 0-9999.

Yes, and if you wanna use accurate LFU, please config set maxmemory-policy *-lfu.

@FNST-zhoush
Copy link
Author

i have already configed maxmemory-policy allkes-lfu and maxmemory 1G.

127.0.0.1:4001> config get maxmemory-policy

  1. "maxmemory-policy"
  2. "allkeys-lfu"
    127.0.0.1:4001> config get maxmemory
  3. "maxmemory"
  4. "1000000000"
    127.0.0.1:4001> flushall
    OK
    127.0.0.1:4001> incr key
    (integer) 1
    127.0.0.1:4001> object freq key
    (integer) 0
    127.0.0.1:4001> incr key
    (integer) 2
    127.0.0.1:4001> object freq key
    (integer) 1
    127.0.0.1:4001> set key2 1
    OK
    127.0.0.1:4001> object freq key2
    (integer) 5

In my point, there is not LFU(on/off) check in createStringObjectFromLongLong.

@ccqy66
Copy link

ccqy66 commented May 24, 2018

@FNST-zhoush

Redis creates shared objects including integer 0-9999.

so we can see redis how to create the shared integer objects from redis-4.0.2

void createSharedObjects(void) {
...
 for (j = 0; j < OBJ_SHARED_INTEGERS; j++) {
        shared.integers[j] =
            makeObjectShared(createObject(OBJ_STRING,(void*)(long)j));
        shared.integers[j]->encoding = OBJ_ENCODING_INT;
  }
...
}
robj *createObject(int type, void *ptr) {
    ...
    if (server.maxmemory_policy & MAXMEMORY_FLAG_LFU) {
        o->lru = (LFUGetTimeInMinutes()<<8) | LFU_INIT_VAL;
    } else {
        o->lru = LRU_CLOCK();
    }
    return o;
}

so,I think if you set maxmemory_policy=allkeys-lfu,lfu counter should be LFU_INIT_VAL.something else,you can provide some extra infomation.for example redis version. or read source code

@FNST-zhoush
Copy link
Author

void incrDecrCommand(client *c, long long incr) {
	...
    if (o && o->refcount == 1 && o->encoding == OBJ_ENCODING_INT &&
        (value < 0 || value >= OBJ_SHARED_INTEGERS) &&
        value >= LONG_MIN && value <= LONG_MAX)
    {
        new = o;
        o->ptr = (void*)((long)value);
    } else {
        new = createStringObjectFromLongLong(value);
        if (o) {
            dbOverwrite(c->db,c->argv[1],new);
        } else {
            dbAdd(c->db,c->argv[1],new);
        }
    }
}

robj *createStringObjectFromLongLong(long long value) {
    robj *o;
    if (value >= 0 && value < OBJ_SHARED_INTEGERS) {
        incrRefCount(shared.integers[value]);
        o = shared.integers[value];
	}else 
	    ...
}

if o doesn't exist,INCR command can create a key by createStringObjectFromLongLong.
but it is a shard object evenif maxmemory-policy configured allkes-lfu and maxmemory=1G.
i think we should avoid using shared integers when maxmemory & maxmemory-policy is used because every object needs to have a private LFU field for the LFU algorithm to work well.

@shenlongxing
Copy link
Contributor

It is a bug, and i open a PR to fix this problem.#5011

@ccqy66
Copy link

ccqy66 commented Jun 12, 2018

@shenlongxing I think the purpose of algorithm of evicting is eliminating extra memory, the pr will create more object,and run algorithm to evict it.so I think it's waste of memory,if it's share object,it should exist all the time.

@shenlongxing
Copy link
Contributor

@ccqy66 I do not agree.
The key point is the newly created object can not be a shared one once maxmemory-policy is set to *-lfu.

@FNST-zhoush
Copy link
Author

@antirez It's a bug in my point. What do you think?

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

No branches or pull requests

5 participants