FEATURE : Add block allocator for more efficient memory management(task/eblock) #113

jooho812 · 2017-07-11T12:13:34Z

This feature is applied only on btree collection.

btree collection 적용 PR입니다

@MinWooJin
review부탁드립니다.

- This feature is applied only on btree collection.

MinWooJin · 2017-07-12T11:25:22Z

memcached.c

@@ -5581,6 +5788,19 @@ static void process_bin_bop_get(conn *c) {
        est_count = req->message.body.count;
        if (est_count % 2) est_count += 1;
    }
+#ifdef USE_EBLOCK_RESULT
+    EBLOCK_MGET_INIT(&c->eblk_ret, 1);


bop mget에서 loop를 돌면서 key 하나씩 get 해오는 부분 때문에 num_keys가 추가되고,
단일 bop get에서 1을 넘겨서 초기화를 하고 있는데요, mget 하나만을 위해서 모든 collection에 이부분이 추가되는게 이상합니다.
bop mget을 smget처럼 key array를 넘기고 engine 내부에서 loop를 돌면서 get 해올 수 있도록 하면 제거가 가능 해 보입니다. 수정하기 위해서는 btree_elem_mget 을 위한 API 추가가 필요할 것 같긴 합니다.

@jooho812 @jhpark816 의견 주시면 좋을 것 같습니다.

사실 이 부분이 mget할때는 그나마 이상하지 않은데 일반적인 get할때는 조금 이상하게 보여서 고민을 하다가 이런형태로 넣었는데 말씀해주신대로 차라리 추가적인 API를 만드는게 더 나아보이긴 하네요.
추가적인 API만드는 쪽으로 작업을 진행할까요?

하나의 eblock result 안에 여러 key들의 element들을 수록한다면,
각 key 별로 몇 개의 element들이 들어 간 것인지 알아야 하는 데,
현재의 eblock result는 이를 표현하지 못합니다.

이를 자연스럽게 표현하도록 eblock result 구조를 가진다면, 아래가 가능합니다.

하나의 eblock result 사용

btree_elem_mget() 함수 필요

위의 불가능하다면, eblock result array 모양의 결과를 만들어야 할 거예요.
이 경우는 아래 두 경우가 모두 가능해요.

N 번의 btree_elem_get() 수행

1 번의 btree_elem_mget() 수행

위의 모든 경우를 살펴보고, 1차 의견 주세요.

제가 잘 이해했는지는 모르겠는데 제 생각을 말씀드리면 eblock result를 array모양의 결과로 가져가게되면 읽을때나 release할때 모두 개별 eblock result에 접근을 해야하고, 여러 키에대한 연산인 mget을 이 형태로 가져가게되면 100개의 키에대한 엘리먼트들을 1개씩 가져올때도 기존에는 1블럭이면 될 수도있는데 블럭을 한개씩 각각 가져가게되서 비효율적이란 생각이 듭니다.

그래서 btree_elem_mget함수를 따로만들어서 내부에서 반복적으로 btree_elem_get을 호출하는 형태로 가져가는게 어떨까하는 생각입니다. process_bop_prepare_nread_keys 함수에서 mget을 수행하면서 필요한 반복작업에 대한 메모리(mget에 대한 response를 만들 때 필요한 flags, trimmed, ret....에 대한 공간)를 smget과 동일한 형태처럼 미리 할당해두고 사용하면 어떨까 합니다.

정리해보면 response에 대한 메모리 할당할때 smget과 동일한형태로 mget에서 필요한 메모리를 함께 할당해두고(flags,trimmed, ret,....) btree_elem_mget API를 새로 만들어서 엔진내부에서 btree_elem_get을 여러번 호출하는 형태로 하는게 나을 것 같다는 생각입니다.

각 key별 element count는 어떻게 유지할 계획인가요 ?

각 key별 element count도 flags,trimmed, ret과 함께 메모리 공간을 미리할당해두고 mget함수에서 변수를 하나두고 btree_elem_get을 호출할때마다 eblock result의 elem_cnt를 확인해서 그 차이를 elem_count의 array에 담아서 사용하면 될 것 같다는 생각입니다.

mget 용 eblock result를 담는 구조체를 정의하여 사용하는 것이 좋겠어요.

넵 그럼 기존 eblock_result처럼 세션에 mget용 eblock_result를 추가해서 사용하는 방향으로 진행 하겠습니다.

single get 용도의 eblock result와
multiple get 용도의 eblock result 공간 중 사이즈가 더 큰 공간 하나만 두고
용도에 따라 type casting하여 사용하면 될 것이에요..
Union 개념 처럼요.

넵 그렇게 진행하겠습니다.

jooho812 · 2017-07-13T02:23:36Z

gbp range타입 변경으로 인한 conflict 사항은 다른 리뷰내용 반영할 때 한번에 반영하도록 하겠습니다.

jooho812 · 2017-07-17T06:15:21Z

btree_elem_mget함수 추가 했습니다. eblock result로 multi get할 때 기존 변수들은 공통적으로 single/multi get에서도 사용이 되어서, 용도에 따른 구분없이 기존 eblock_result_t 구조체에 multi용도로 사용할 때 필요한 변수들을 추가하고 mget용으로 추가한 변수들은 mget할때만 사용하는 형태로 작성했습니다.

MinWooJin · 2017-07-20T08:34:24Z

memcached.c

+        }
+
+        ret = mc_engine.v1->btree_elem_mget(mc_engine.v0, c,
+                                           key_tokens, &c->coll_bkrange,


eblk_ret 구조체에 key count 정보를 가지고 있지만, mget API호출 시 smget 처럼 key count 값을 넘겨주고
내부에서 key count를 얻어오는 동작 없이 loop를 동작하는게 더 나을 것 같습니다.

그게 더 직관적으로 보일 것 같네요 수정해 두겠습니다.

MinWooJin · 2017-07-20T09:47:01Z

memcached.c

+
+                if (ret == ENGINE_SUCCESS) {
+                    sprintf(resultptr, " %s %u %u\r\n",
+                            (c->eblk_ret.trimmed[k]==false ? "OK" : "TRIMMED"), htonl(c->eblk_ret.flags[k]), (c->eblk_ret.mecnt[k]));


변수가 한번에 보고 이해하는데 어렵게 읽히는 것 같습니다.
기존 변수명을 사용할 수 있도록 위에서 한번 담아서 사용하거나, 좀 더 의미가 잘 들어났으면 좋겠습니다.
mecnt naming은 변수 명만 보고 어떤 의미인지 잘 모르겠습니다.

말씀해주신내용 고려해서 수정하겠습니다.

jooho812 · 2017-07-20T12:04:45Z

반영했습니다.

MinWooJin

1차 리뷰 완료 했습니다.

MinWooJin · 2017-07-21T07:19:45Z

engines/default/default_engine.c

+#ifdef USE_EBLOCK_RESULT
+static ENGINE_ERROR_CODE
+default_btree_elem_mget(ENGINE_HANDLE* handle, const void* cookie,
+                        const token_t *key_tokens,


numkeys는 key_tokens 바로다음에 오는게 나을 것 같습니다.

MinWooJin · 2017-07-21T07:20:21Z

engines/default/default_engine.c

+    ENGINE_ERROR_CODE ret;
+    VBUCKET_GUARD(engine, vbucket);
+
+    ret = btree_elem_mget(engine, key_tokens, bkrange, efilter,


이부분도 key_tokens 다음 numkeys가 나오는게 나을 것 같습니다.

MinWooJin · 2017-07-21T08:14:27Z

engines/default/items.c

@@ -4848,24 +4900,41 @@ static int do_btree_elem_batch_get(btree_elem_posi posi, const int count,

        elem = BTREE_GET_ELEM_ITEM(posi.node, posi.indx);
        elem->refcount++;
+#ifdef USE_EBLOCK_RESULT
+        if (reverse) eblk_add_elem_with_posi(eblk_ret, elem, position+count-nfound-1);


position+count-nfound-1이 reverse일 때 역으로 저장하기 위해서 위치를 계산하는 부분인데,
조금 더 잘 포장된 형태로 변경하면 이해하는데 더 좋을 것 같습니다.
딱히 좋은 변경이 현재 떠오르진 않는데, 같이 고민해볼 필요가 있겠습니다.

말씀해주신내용 고려하면서 다시 봤는데 position이 필요 없을 것 같아서 삭제하였습니다.
reverse진행하고 나서 블럭내의 elem_count를 구조체내에 가지고 있고 element를 넣어 줄때마다 증가시켜서 따로 넘겨주지 않고 사용해도 될 것 같습니다.

jooho812 · 2017-07-24T11:19:03Z

리뷰내용 반영했습니다.

MinWooJin

리뷰 완료 했습니다.
smget쪽 정리가 필요할 듯 한데,
어떤방법이 좋을지 딱히 떠오르진 않네요.

MinWooJin · 2017-07-25T05:55:46Z

memcached.c

+#ifdef JHPARK_OLD_SMGET_INTERFACE
+      if (c->coll_smgmode == 0) {
+        int smget_count = c->coll_roffset + c->coll_rcount;
+        int kfnd_array_size;


변수에 대한 설명이 빠져 있습니다.

MinWooJin · 2017-07-25T06:17:23Z

memcached.c

+        int respon_hdr_size; /* the size of response head and tail */
+        int respon_bdy_size; /* the size of response body */
+
+        kfnd_array_size = smget_count * (2*sizeof(uint32_t));


smget_count * (2*sizeof(uint32_t))가 어느 부분을 표현하고 있는 건가요?

기존 -> elem_array_size = smget_count * (sizeof(eitem*) + (2 * sizeof(uint32_t)));
변경 -> kfnd_array_size = smget_count * (2 * sizeof(uint32_t));
로 변경하였는데
기존에는 element array, key find array, flag array에 대한 size를 한번에 계산해서 넣었는데

mblock allocator를 적용하면서 element array에 대한 size만 필요 없어져서 그 부분만 빼고 변수명을 kfnd_array_size로 변경하였습니다.

jooho812 · 2017-08-16T05:26:48Z

@jhpark816
review 부탁드립니다.

jhpark816

우선 bop mget 연산에 대해
먼저 review 의견을 전달해야 할 것 같아
코멘트 남깁니다.

jhpark816 · 2017-08-17T11:11:00Z

memcached.c

+
+        eitem *elem;
+        eblock_scan_t eblk_sc;
+        EBLOCK_SCAN_INIT(&c->eblk_ret, &eblk_sc);


여기서 EBLOCK_SCAN_INIT() 수행은 의미가 없어요.

이러한 부분을 code review에서 지적하는 것은 문제가 있다고 생각되어요.

jhpark816 · 2017-08-17T12:03:53Z

memcached.c

+                                           key_tokens, c->coll_numkeys, &c->coll_bkrange,
+                                           (c->coll_efilter.ncompval==0 ? NULL : &c->coll_efilter),
+                                           c->coll_roffset, c->coll_rcount,
+                                           &c->eblk_ret, &tot_access_count, 0);


eblock_result 사용 방식이 어색하여 btree_elem_mget()을 새로 구현한 것으로 알고 있는 데,
btree_elem_mget()에서도 여전히 eblock_result 사용 방식이 복잡하고 명쾌하지 않은 것 같아요.
또한, 조회 count가 0인 경우, max_btree_size * num_keys 개의 element를 담기 위한
eblock을 prepare하는 것도 큰 문제임. (이 문제를 해결하기 위한 목적도 있는 데,
이 문제가 그대로 남아 있음)

오히려, 아래와 같이 각 key별로 btree_elem_get()을 호출하는 방식이 더 나은 것 같아요.
대신, eblk_result와 eblk_scan 사용 방식은 조금 수정이 필요해요.

eblk_scan_init = false; for (k = 0; k < c->coll_numkeys; k++) { ret = mc_engine.v1->btree_elem_get(mc_engine.v0, c, key_tokens[k].value, key_tokens[k].length, &c->coll_bkrange, (c->coll_efilter.ncompval==0 ? NULL : &c->coll_efilter), c->coll_roffset, c->coll_rcount, false, false, &c->eblk_ret, &cur_access_count, &flags, &trimmed, 0); if (ret == ENGINE_SUCCESS) { cur_elem_count = EBLOCK_ELEM_COUNT(&c->eblk_ret) - tot_elem_count; tot_elem_count = EBLOCK_ELEM_COUNT(&c->eblk_ret); /* prepare io */ if (eblk_scan_init == false) { EBLOCK_SCAN_INIT(&eblk_sc, &c->eblk_ret); eblk_scan_init = true; } else { EBLOCK_SCAN_RESET(&eblk_sc, &c->eblk_ret, cur_elem_count); } for (e = 0; e < cur_elem_count; e++) { EBLOCK_SCAN_ENXT(&eblk_sc, elem); /* prepare io */ } } else { /* prepare io */ } }

eblk_result 사용 방식의 수정

eblk_prepare 수정 필요

eblk_result가 항상 empty 상태가 아닌 상태에서 prepare 요청할 수 있다는 점.

즉, element를 추가로 append하기 위하여 prepare 할 수 있게 수정

eblk_add_elem과 eblk_truncate는 그대로

eblk_scan 사용 방식의 수정 : "element 추가 => scan 작업"이 이어서 반복될 수 있게.

EBLOCK_SCAN 구조체 의미 변경

scan pointer (mblock pointer와 그 mblock 내에서 element index)

scan 해야 할 element count

EBLOCK_SCAN_RESET 추가

현재의 scan pointer는 그대로 두고, scan할 element count만 새로 변경.

EBLOCK_SCAN_NEXT

현재의 scan pointer 위치에서 이어서 scan 작업 수행

저번에 조언해주셨던 내용중에 연속적인 operation작업을 하면서 그때그때 블록을 받아서 처리하게되면 한연산내에서 어떤연산은 수행되고 어떤연산은 블록을 받아오는작업이 실패하여 일부연산만 되는건 이상하다라고 하셔서 일단 전체 연산에 대한 블록을 미리 받아놓는쪽으로 생각하고 진행하였는데

코멘트해주신 내용대로 진행하게되면 위와 같은 상황이 발생할 수 있을 것 같은데 말씀해주신대로 진행할까요?

@jooho812 block을 할당받는 작업이 실패하는 이유 때문에 bop mget() 연산을 추가한 것인가요 ?
기존 코드에서도 prepare io 하면서 memory 부족으로 실패할 수 있어요.

jooho812 · 2017-08-18T09:33:52Z

말씀해주신 내용 반영했습니다.

bop_mget operation 삭제
- bop_elem_get 반복
eblk_prepare 수정
- 모든 collection operation의 동작은 eblk_prepare -> eblk_truncate -> free순으로 이루어진다.
- eblk_prepare에서 empty상태여부를 eblk_ret->head_blk로 판단하게끔 변경. eblk_ret->head_blk를 free시마다 NULL로 초기화하여 eblk_prepare시에 empty여부를 확인
eblk_scan사용 방식 수정
- 기존동작은 초기화후에 scan->idx에 해당하는 아이템을 가져온 후 다음블록으로 갈지 여부를 정했는데 scan->idx가 블록의 마지막일경우 next block은 null이 된다.
- element get -> next block 여부 에서 next block 여부 -> element get 순서로 바뀜

MinWooJin · 2017-08-18T09:59:10Z

engines/COMMON/mblock_allocator.c

+        blkcnt = ((result->elem_cnt + elem_count - 1) / EITEMS_PER_BLOCK) + 1;
+        if (blkcnt > curr_blkcnt) { // need append block
+            if (!mblock_list_alloc((blkcnt - curr_blkcnt), &tmp_head, &tmp_last)) {
+                result->elem_cnt = curr_elemcnt;


mblock_list_alloc()이 실패하면, result->elem_cnt가 변경이 되나요??

아니요 변경되지 않습니다. 삭제해두겠습니다.

MinWooJin · 2017-08-18T10:01:13Z

engines/COMMON/mblock_allocator.c

+        result->tail_blk = NULL;
+    } else {
+        mem_block_t *tmp_head;
+        mem_block_t *tmp_last;


tmp_head, tmp_last 보다는 haed, last로 해도 무방할 것 같습니다.

jhpark816

현재 review 한 것만 comment 남겨요.

jhpark816 · 2017-08-18T09:56:52Z

engines/default/items.c

@@ -4907,6 +4993,10 @@ static ENGINE_ERROR_CODE do_btree_elem_get_by_posi(btree_meta_info *info,

    if (info->root == NULL) return ENGINE_ELEM_ENOENT;

+#ifdef USE_EBLOCK_RESULT
+    if (!eblk_prepare(eblk_ret, (count > 0 && count < info->ccnt) ? count : info->ccnt))


이 함수에서 count 인자 값은 항상 0보다 큽니다.
아래 조건이면 충분합니다.

((count < info->ccnt) ? count : info->ccnt)

jhpark816 · 2017-08-18T10:00:25Z

engines/COMMON/mblock_allocator.c

            return false;
        }
    }

    return true;
 }

-void mblock_list_free(uint32_t blck_cnt, mem_block_t *head_blk, mem_block_t *tail_blk) {
+void mblock_list_free(uint32_t blck_cnt, mem_block_t **head_blk, mem_block_t **tail_blk) {


mblock_list_free() 함수에서 head_blk와 tail_blk의 data type을 변경할 이유가 없어 보입니다.

바꾼 의도는 mget같은 연속적인 operation에서 기존 블록에 새 블록을 append할지 아니면 새로운 리스트를 받을지에 대한 판단을 head_blk가 NULL인지 여부로 판단하려고 했고

모든 operation이 eblk_prepare를 통해 블록리스트를 받은 후에는 mblock_list_free를 호출해서 여기서 head_blk를 NULL으로 반환하면 되겠다는 생각때문에 수정했는데 제가 잘못생각한걸까요

아래 글의 의미가 무엇 인지를 분명하게 설명해 주면 좋겠어요.

모든 operation이 eblk_prepare를 통해 블록리스트를 받은 후에는 mblock_list_free를 호출해서 여기서 head_blk를 NULL으로 반환하면 되겠다는 생각

내 의견은

mblock_list_alloc()은 mblock list를 allocate하는 함수라서,
list의 head와 tail pointer를 받아오기 위해 mem_block_t ** 타입이 필요해요.

mblock_list_free()는 mblock list를 free하는 함수라서,
list의 head와 tail pointer 자체를 넘겨주면 되므로, mem_block_t *타입이면 충분해요.

일단 수정하면서 multiget과 get의 차이에 대한 판단을 mblock_prepare를 통해 넘어온 eblock_result_t 구조체의 head_blk를 보고 새로운 mblock list를 받거나, 기존 블록 리스트에 더 필요한 mblock들을 append하는 형태대로 수정을 하려고 했고

기존

mblock_list_free()함수는 말씀해주신것처럼 free하는 목적으로 head, tail pointer만 넘겨서 반환

변경

의도는 eblk_ret->head_blk를 operation이 끝난 후 NULL로 초기화 해줘야하는데 마땅한 위치를 찾기보다는 get operation 이후엔 모두 free과정을 거치니까 mblock_list_free()함수 내에서 바꿔주면 좋겠다고 판단을 했습니다. mblock_allocator.c 파일내에서 호출되는 경우엔 불필요하긴한데 default engine에서 호출된 mblock_list_free()의 경우 eblk_ret->head_blk를 null로 반환해 주어야 하기때문에 이렇게 수정했습니다.

prepare 단계에서 block을 붙여서 받을지에 대한 판단을 head_blk == NULL로 하고 있기 때문에,
free하는 단계에서 NULL setting을 해주기 위해선 @jooho812 의견처럼 mem_block_t ** 타입으로
해야될 것 같은데요, 혹시 더 괜찮은 의견이 있으신가요?

만약 free 함수 내부에서 NULL setting을 하지 않는다면, free 함수 호출하고 나서 NULL setting을 밖에서 해주어야 합니다.

jhpark816 · 2017-08-18T10:02:49Z

engines/COMMON/mblock_allocator.c

+        result->elem_cnt = 0;
+    }
+    result->tail_blk->items[posi % EITEMS_PER_BLOCK] = (eitem *)elem;
+    result->elem_cnt++;


posi 라는 값이
eblock result에 들어가는 전체 element들에서의 위치 값이기 때문에,
어떤 mblock과 그 mblock 내에서의 index 값이 계산되어야 합니다.

위 코드는 어떤 mblock인지를 찾지 않고
단순히 tail block에 넣는 것을 가정하고 있네요.

이 함수를 만든게 bop pwg연산 때문에 만들었고 이 연산에서 validation check할때 element 갯수를 100개로 제한하고 있어서 현재는 어떤 mblock인지 찾을 필요없이 그냥 넣으면 될 것 같아서 주석과 assert문만 넣어두었는데 어떤 mblock인지 찾는걸 추가해 둘까요?

예. 어떤 mblock인지를 확인하는 것이 보다 안전해 보여요.

반영해두겠습니다.

jhpark816 · 2017-08-18T10:06:24Z

engines/default/items.c

@@ -4876,6 +4935,22 @@ static int do_btree_posi_find_with_get(btree_meta_info *info,
        ecnt = 1;                             /* elem count */
        eidx = (bpos < count) ? bpos : count; /* elem index in elem array */
        elem->refcount++;
+#ifdef USE_EBLOCK_RESULT
+        if (!eblk_prepare(eblk_ret, (eidx + count + 1)))
+            return ENGINE_ENOMEM;


eblock prepare & truncate 위치를 재조정하세요.

현재는 위에서 찾은 element에 대해 refcount를 증가시킨 후에
eblock prepare가 실패하면 바로 return하고 있네요.

MinWooJin

몇가지 추가로 리뷰 진행 했습니다.

MinWooJin · 2017-08-19T03:48:36Z

engines/COMMON/mblock_allocator.c

+        uint32_t curr_blkcnt = result->blck_cnt;
+        blkcnt = ((result->elem_cnt + elem_count - 1) / EITEMS_PER_BLOCK) + 1;
+        if (blkcnt > curr_blkcnt) { // need append block
+            if (!mblock_list_alloc((blkcnt - curr_blkcnt), &head, &last))


필요한 block count를 먼저 계산 해 놓고,
필요한 block이 있으면(> 0) count만 넘겨서 list_alloc을 하면 더 좋을 것 같습니다.

MinWooJin · 2017-08-19T08:51:35Z

memcached.c

@@ -2262,9 +2324,25 @@ static void process_bop_mget_complete(conn *c) {
                }
                resultptr += strlen(resultptr);

+#ifdef USE_EBLOCK_RESULT
+                if (cur_elem_count > 0) {


cur_elem_count 가 0인 경우가 있나요??

0인 경우가 없네요 수정해두겠습니다.

MinWooJin · 2017-08-19T09:07:38Z

memcached.c

+#ifdef USE_EBLOCK_RESULT
+                if (cur_elem_count > 0) {
+                    if (eblk_scan_init == false) {
+                        EBLOCK_SCAN_INIT(&c->eblk_ret, &eblk_sc);


질문입니다.
elem_get 이전에 dummy로 INIT을 해주고,
elem_get 이후에 내부에서 INIT을 해주는 이유는 어떤건가요?

이렇게 해둔의도는 eblk_sc를 초기화안해줘서 컴파일할때 에러가나고 그걸 피하려고 위에 따로 의미없는 초기화를 해두었는데 이럴경우에 좀 더 좋은 방안이 있을까요.

jhpark816 · 2017-08-23T06:10:06Z

@jooho812 review 수정 완료되면, @MinWooJin 에게 review 요청해 주세요.

jooho812 · 2017-08-23T09:36:24Z

반영했습니다.

@MinWooJin review 부탁드립니다.

MinWooJin

리뷰 완료 했습니다.

MinWooJin · 2017-08-25T01:09:17Z

engines/default/items.c

                                           uint32_t *access_count, bool *potentialbkeytrim)
 {
+


이부분 line은 없어도 될 것 같습니다.

MinWooJin · 2017-08-25T01:16:36Z

engines/default/items.c

@@ -4486,7 +4511,12 @@ static ENGINE_ERROR_CODE do_btree_elem_get(struct default_engine *engine, btree_
            if (offset == 0) {
                if (efilter == NULL || do_btree_elem_filter(elem, efilter)) {
                    elem->refcount++;
+#ifdef USE_EBLOCK_RESULT
+                    eblk_add_elem(eblk_ret, elem);
+                    tot_found++;


eblock_result_t 구조체 내부에 elem_count라는 변수가
tot_found와 같은 의미로 사용되고 있는데요, 삭제해도 괜찮을 것 같습니다.

말씀해주신대로 삭제해도 괜찮긴한데, tot_found를 삭제하면 아래쪽에 tot_found명칭으로 사용되고 있는 부분들을 전부 수정을 해야하는데 소스가 지저분해질 것 같아서 내버려두었습니다.

MinWooJin · 2017-09-11T02:49:13Z

engines/COMMON/mblock_allocator.c

-        result->elem_cnt = 0;
-        return false;
+    uint32_t blkcnt;
+    if (result->head_blk == NULL) { // empty block


head_blk이 NULL인 것으로 판단하는 것 보다
flag를 하나 두고 alloc 되었는지 또는 appendMode인지
판단하는게 이해하는데 더 나을 것 같은데 어떤가요?

제가 저렇게 한 의도는 두가지인데

한가지는 appendMode인지 여부를 확인하기 위함도 있고

다른 한가지는 기존에 c->eblk_ret에 블록리스트를 받아 사용후 free해도 아직 블록의 유의미한 주소값이 남아 있게되는데 null로 초기화해주기 위함도 있습니다.

말씀해주신대로 flag를 추가하면 이해하는 측면에서는 더 나아보일 것 같은데, 지금 형태도 나쁘지는 않게 생각이 됩니다.
바꾸는게 나을까요?

MinWooJin · 2017-09-11T03:17:45Z

memcached.c

+        if ((bkeyptr = (uint64_t *)malloc(need_size)) == NULL) {
+            ret = ENGINE_ENOMEM;
+        } else {
+            vlenptr = (uint32_t *)((char*)bkeyptr + (sizeof(uint64_t) * elem_count));


binary 부분이라서 리뷰하는게 조금 그렇긴 하지만,
아래에 info value의 선언을 위로 올려서 모아서 선언하는게 나을 것 같습니다.

MinWooJin · 2017-09-12T03:00:21Z

include/memcached/types.h

 #define EBLOCK_SCAN_NEXT(s, e)                                               \
    do {                                                                     \
        if ((s)->idx < (s)->tot) {                                           \
-            (e) = ((s)->blk)->items[(s)->idx % EITEMS_PER_BLOCK];            \
-            if ((((s)->idx)++ % EITEMS_PER_BLOCK) == (EITEMS_PER_BLOCK - 1)) \
+            if (((s)->idx % EITEMS_PER_BLOCK) == 0 && ((s)->idx != 0))       \


아래 문장 때문에 { }로 묶어주는게 나을 것 같습니다.

jooho812 · 2017-09-27T02:01:26Z

#129 PR에서 이어서 진행하므로 본 PR은 close하겠습니다.

FEATURE : Add block allocator for moere efficient memory management

06d95da

- This feature is applied only on btree collection.

jooho812 force-pushed the jooho812/block_allocator branch from 871ea80 to 06d95da Compare July 11, 2017 12:44

MinWooJin reviewed Jul 12, 2017

View reviewed changes

jooho812 added 3 commits July 14, 2017 15:13

CLEANUP : change the range type of bop gbp

2c3285b

CLEANUP : add btree_elem_mget

2de0288

Merge branch 'task/eblock' into jooho812/block_allocator

1ba8c5c

MinWooJin reviewed Jul 20, 2017

View reviewed changes

CLEANUP : reflected minwoojin reviews

bd09794

MinWooJin reviewed Jul 21, 2017

View reviewed changes

CLEANUP : reflected minwoojin review

5cb240b

MinWooJin reviewed Aug 16, 2017

View reviewed changes

CLEANUP : reflected minwoojin review3

c110481

jhpark816 reviewed Aug 17, 2017

View reviewed changes

CLEANUP : reflected jhpark816 review

9d37f89

jooho812 force-pushed the jooho812/block_allocator branch from bd57797 to 9d37f89 Compare August 18, 2017 09:14

MinWooJin reviewed Aug 18, 2017

View reviewed changes

CLEANUP : reflected minwoojin review

6341ad4

jooho812 changed the title ~~FEATURE : Add block allocator for moere efficient memory management(task/eblock)~~ FEATURE : Add block allocator for more efficient memory management(task/eblock) Aug 18, 2017

jhpark816 reviewed Aug 18, 2017

View reviewed changes

MinWooJin reviewed Aug 19, 2017

View reviewed changes

jooho812 force-pushed the jooho812/block_allocator branch from 6898231 to fb5dec5 Compare August 21, 2017 02:55

CLEANUP : reflected reviews

88bdb04

jooho812 force-pushed the jooho812/block_allocator branch from fb5dec5 to 88bdb04 Compare August 21, 2017 05:01

jhpark816 assigned jooho812 Aug 23, 2017

CLEANUP : modified eblk_add_elem_with_posi

17aaf07

jooho812 force-pushed the jooho812/block_allocator branch from ba7dff2 to 17aaf07 Compare August 23, 2017 09:05

MinWooJin reviewed Sep 12, 2017

View reviewed changes

CLEANUP : reflected minwoojin review

c8e71d8

jooho812 force-pushed the jooho812/block_allocator branch from b7b9899 to c8e71d8 Compare September 15, 2017 05:32

jooho812 force-pushed the jooho812/block_allocator branch 2 times, most recently from 9debcf5 to c8e71d8 Compare September 26, 2017 03:23

jooho812 mentioned this pull request Sep 26, 2017

FEATURE : Add block allocator for more efficient memory management #129

Open

2 tasks

jooho812 closed this Sep 27, 2017

jooho812 deleted the jooho812/block_allocator branch December 21, 2017 05:37

FEATURE : Add block allocator for more efficient memory management(task/eblock) #113

FEATURE : Add block allocator for more efficient memory management(task/eblock) #113

Conversation

jooho812 commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 commented Jul 13, 2017

jooho812 commented Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 commented Jul 20, 2017

MinWooJin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

jooho812 commented Jul 24, 2017

MinWooJin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

jooho812 commented Aug 16, 2017

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinWooJin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Aug 23, 2017

jooho812 commented Aug 23, 2017

MinWooJin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jooho812 Jul 13, 2017 •

edited

Loading

jooho812 Jul 14, 2017 •

edited

Loading

jooho812 commented Jul 17, 2017 •

edited

Loading

jooho812 Jul 24, 2017 •

edited

Loading

jooho812 Aug 16, 2017 •

edited

Loading

jooho812 Sep 12, 2017 •

edited

Loading