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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 12 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
06d95da
FEATURE : Add block allocator for moere efficient memory management
jooho812 Jul 11, 2017
2c3285b
CLEANUP : change the range type of bop gbp
jooho812 Jul 14, 2017
2de0288
CLEANUP : add btree_elem_mget
jooho812 Jul 17, 2017
1ba8c5c
Merge branch 'task/eblock' into jooho812/block_allocator
jooho812 Jul 17, 2017
bd09794
CLEANUP : reflected minwoojin reviews
jooho812 Jul 20, 2017
5cb240b
CLEANUP : reflected minwoojin review
jooho812 Jul 24, 2017
c110481
CLEANUP : reflected minwoojin review3
jooho812 Aug 16, 2017
9d37f89
CLEANUP : reflected jhpark816 review
jooho812 Aug 18, 2017
6341ad4
CLEANUP : reflected minwoojin review
jooho812 Aug 18, 2017
88bdb04
CLEANUP : reflected reviews
jooho812 Aug 21, 2017
17aaf07
CLEANUP : modified eblk_add_elem_with_posi
jooho812 Aug 23, 2017
c8e71d8
CLEANUP : reflected minwoojin review
jooho812 Sep 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 42 additions & 14 deletions engines/COMMON/mblock_allocator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,32 +171,33 @@ bool mblock_list_alloc(uint32_t blck_cnt, mem_block_t **head_blk, mem_block_t **
total_mblocks += new_cnt;
//pthread_mutex_unlock(&pool_mutex);
if (alloc_cnt < blck_cnt) {
mblock_list_free(alloc_cnt, *head_blk, *tail_blk);
mblock_list_free(alloc_cnt, head_blk, tail_blk);
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

모든 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 *타입이면 충분해요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

일단 수정하면서 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로 반환해 주어야 하기때문에 이렇게 수정했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

//mem_block_t *bye = NULL;
//mem_block_t *bye_helper = NULL;

//pthread_mutex_lock(&pool_mutex);
if (head_blk == NULL || blck_cnt == 0)
if (*head_blk == NULL || blck_cnt == 0)
return;

assert(pool_tail == NULL || pool_tail->next == NULL);
assert(tail_blk->next == NULL);
assert((*tail_blk)->next == NULL);

if (pool_head == NULL) {
pool_head = head_blk;
pool_head = *head_blk;
} else {
pool_tail->next = head_blk;
pool_tail->next = *head_blk;
}
pool_tail = tail_blk;
pool_tail = *tail_blk;

*head_blk = *tail_blk = NULL;
free_mblocks += blck_cnt;
assert(free_mblocks <= total_mblocks);

Expand All @@ -223,17 +224,33 @@ void mblock_list_free(uint32_t blck_cnt, mem_block_t *head_blk, mem_block_t *tai
free(bye_helper);
}*/
}

bool eblk_prepare(eblock_result_t *result, uint32_t elem_count) {
assert(elem_count > 0);
uint32_t blkcnt = ((elem_count - 1) / EITEMS_PER_BLOCK) + 1;
if (!mblock_list_alloc(blkcnt, &result->head_blk, &result->last_blk)) {
result->elem_cnt = 0;
return false;
uint32_t blkcnt;
if (result->head_blk == NULL) { // empty block
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jooho812 jooho812 Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

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

blkcnt = ((elem_count - 1) / EITEMS_PER_BLOCK) + 1;
if (!mblock_list_alloc(blkcnt, &result->head_blk, &result->last_blk)) {
result->elem_cnt = 0;
return false;
}
result->tail_blk = NULL;
} else {
mem_block_t *head;
mem_block_t *last;
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

return false;
result->last_blk->next = head;
result->last_blk = last;
}
}
result->tail_blk = NULL;
result->blck_cnt = blkcnt;
return true;
}

void eblk_truncate(eblock_result_t *result) {
assert(result->last_blk->next == NULL);
/* returns empty blocklist */
Expand All @@ -244,13 +261,13 @@ void eblk_truncate(eblock_result_t *result) {
uint32_t used_nblks = ((result->elem_cnt - 1) / EITEMS_PER_BLOCK) + 1;
uint32_t free_nblks = result->blck_cnt - used_nblks;

mblock_list_free(free_nblks, free_head, free_tail);
mblock_list_free(free_nblks, &free_head, &free_tail);
result->tail_blk->next = NULL;
result->last_blk = result->tail_blk;
result->blck_cnt -= free_nblks;
}
} else { /* ENGINE_ELEM_ENOENT case */
mblock_list_free(result->blck_cnt, result->head_blk, result->last_blk);
mblock_list_free(result->blck_cnt, &result->head_blk, &result->last_blk);
result->head_blk = result->tail_blk = result->last_blk = NULL;
result->elem_cnt = result->blck_cnt = 0;
}
Expand All @@ -268,3 +285,14 @@ void eblk_add_elem(eblock_result_t *result, eitem *elem) {

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

void eblk_add_elem_with_posi(eblock_result_t *result, eitem *elem, int posi) {
/* This function should not be used when there are multiple blocks. */
assert(result->blck_cnt == 1);
if (result->tail_blk == NULL) {
result->tail_blk = result->head_blk;
result->elem_cnt = 0;
}
result->tail_blk->items[posi % EITEMS_PER_BLOCK] = (eitem *)elem;
result->elem_cnt++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영해두겠습니다.

}
4 changes: 2 additions & 2 deletions engines/COMMON/mblock_allocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ void mblock_allocator_destroy(void);
void mblock_allocator_stats(mblock_stats *blk_stat);

bool mblock_list_alloc(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);
void mblock_list_free(uint32_t blck_cnt, mem_block_t **head_blk, mem_block_t **tail_blk);

bool eblk_prepare(eblock_result_t *result, uint32_t elem_count);
void eblk_truncate(eblock_result_t *result);
void eblk_add_elem(eblock_result_t *result, eitem *elem);

void eblk_add_elem_with_posi(eblock_result_t *result, eitem *elem, int posi);
#endif
47 changes: 47 additions & 0 deletions engines/default/default_engine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,13 +777,23 @@ default_btree_elem_alloc(ENGINE_HANDLE* handle, const void* cookie,
return ret;
}

#ifdef USE_EBLOCK_RESULT
static void
default_btree_elem_release(ENGINE_HANDLE* handle, const void *cookie,
eitem *eitem, EITEM_TYPE type)
{
struct default_engine *engine = get_handle(handle);
btree_elem_release(engine, eitem, type);
}
#else
static void
default_btree_elem_release(ENGINE_HANDLE* handle, const void *cookie,
eitem **eitem_array, const int eitem_count)
{
struct default_engine *engine = get_handle(handle);
btree_elem_release(engine, (btree_elem_item**)eitem_array, eitem_count);
}
#endif

static ENGINE_ERROR_CODE
default_btree_elem_insert(ENGINE_HANDLE* handle, const void* cookie,
Expand Down Expand Up @@ -879,7 +889,11 @@ default_btree_elem_get(ENGINE_HANDLE* handle, const void* cookie,
const bkey_range *bkrange, const eflag_filter *efilter,
const uint32_t offset, const uint32_t req_count,
const bool delete, const bool drop_if_empty,
#ifdef USE_EBLOCK_RESULT
eblock_result_t *eblk_ret,
#else
eitem** eitem_array, uint32_t* eitem_count,
#endif
uint32_t *access_count, uint32_t* flags,
bool* dropped_trimmed, uint16_t vbucket)
{
Expand All @@ -890,7 +904,11 @@ default_btree_elem_get(ENGINE_HANDLE* handle, const void* cookie,
if (delete) ACTION_BEFORE_WRITE(cookie, key, nkey);
ret = btree_elem_get(engine, key, nkey, bkrange, efilter,
offset, req_count, delete, drop_if_empty,
#ifdef USE_EBLOCK_RESULT
eblk_ret,
#else
(btree_elem_item**)eitem_array, eitem_count,
#endif
access_count, flags, dropped_trimmed);
if (delete) ACTION_AFTER_WRITE(cookie, ret);
return ret;
Expand Down Expand Up @@ -932,17 +950,27 @@ default_btree_posi_find_with_get(ENGINE_HANDLE* handle, const void* cookie,
const char *key, const size_t nkey,
const bkey_range *bkrange,
ENGINE_BTREE_ORDER order, const uint32_t count,
#ifdef USE_EBLOCK_RESULT
int *position, eblock_result_t *eblk_ret,
uint32_t *eitem_index,
#else
int *position, eitem **eitem_array,
uint32_t *eitem_count, uint32_t *eitem_index,
#endif
uint32_t *flags, uint16_t vbucket)
{
struct default_engine *engine = get_handle(handle);
ENGINE_ERROR_CODE ret;
VBUCKET_GUARD(engine, vbucket);

ret = btree_posi_find_with_get(engine, key, nkey, bkrange, order, count,
#ifdef USE_EBLOCK_RESULT
position, eblk_ret,
eitem_index, flags);
#else
position, (btree_elem_item**)eitem_array,
eitem_count, eitem_index, flags);
#endif
return ret;
}

Expand All @@ -951,15 +979,23 @@ default_btree_elem_get_by_posi(ENGINE_HANDLE* handle, const void* cookie,
const char *key, const size_t nkey,
ENGINE_BTREE_ORDER order,
uint32_t from_posi, uint32_t to_posi,
#ifdef USE_EBLOCK_RESULT
eblock_result_t *eblk_ret,
#else
eitem **eitem_array, uint32_t *eitem_count,
#endif
uint32_t *flags, uint16_t vbucket)
{
struct default_engine *engine = get_handle(handle);
ENGINE_ERROR_CODE ret;
VBUCKET_GUARD(engine, vbucket);

ret = btree_elem_get_by_posi(engine, key, nkey, order, from_posi, to_posi,
#ifdef USE_EBLOCK_RESULT
eblk_ret,
#else
(btree_elem_item**)eitem_array, eitem_count,
#endif
flags);
return ret;
}
Expand All @@ -973,10 +1009,16 @@ default_btree_elem_smget_old(ENGINE_HANDLE* handle, const void* cookie,
const bkey_range *bkrange,
const eflag_filter *efilter,
const uint32_t offset, const uint32_t count,
#ifdef USE_EBLOCK_RESULT
eblock_result_t *eblk_ret,
uint32_t* kfnd_array,
uint32_t* flag_array,
#else
eitem** eitem_array,
uint32_t* kfnd_array,
uint32_t* flag_array,
uint32_t* eitem_count,
#endif
uint32_t* missed_key_array,
uint32_t* missed_key_count,
bool *trimmed, bool *duplicated,
Expand All @@ -987,8 +1029,13 @@ default_btree_elem_smget_old(ENGINE_HANDLE* handle, const void* cookie,
VBUCKET_GUARD(engine, vbucket);

ret = btree_elem_smget_old(engine, karray, kcount, bkrange, efilter,
#ifdef USE_EBLOCK_RESULT
offset, count, eblk_ret,
kfnd_array, flag_array,
#else
offset, count, (btree_elem_item**)eitem_array,
kfnd_array, flag_array, eitem_count,
#endif
missed_key_array, missed_key_count,
trimmed, duplicated);
return ret;
Expand Down
Loading