INTERNAL: Change tot_elem_cnt to subtree element count in set #817

jeesup0103 · 2025-01-08T06:09:34Z

🔗 Related Issue

https://github.com/jam2in/arcus-works/issues/571

⌨️ What I did

Set에서 tot_elem_cnt를 해당 노드의 subtree에 포함된 모든 element의 총 개수로 변경
- 기존에는 현재 노드의 element 개수만 의미했으나, subtree 전체를 포함하도록 수정하였습니다.

세부 구현 내용

tot_elem_cnt 크기 확장
- 기존 uint16_t 에서 uint32_t로 변경
- Set의 최대 element 개수(1,000,000개)를 표현하기 위해서입니다.
tot_elem_cnt 변수 관리
- Element Insert (do_set_elem_link())
  - Parent 노드들과 현재 노드의 tot_elem_cnt를 1 증가
- Element Delete (do_set_elem_traverse_dfs(), do_set_elem_traverse_delete())
  - DFS 탐색하면서 tot_elem_cnt를 1 감소
- Chain -> Hash table 변경 (do_set_node_link())
  - Parent 노드의 tot_elem_cnt 유지
  - Child 노드의 tot_elem_cnt = 기존 Chain element 개수 (num_found)
- Hash table -> Chain (do_set_node_unlink())
  - Parent 노드의 tot_elem_cnt 유지
  - Child 노드의 tot_elem_cnt = 0

추후 PR 내용

offset 내부 구현 PR
random 구현 PR (parsing, 내부)

jhpark816 · 2025-01-09T09:27:48Z

engines/default/item_base.h

@@ -236,7 +236,7 @@ typedef struct _set_hash_node {
    uint16_t refcount;
    uint8_t  slabs_clsid;         /* which slab class we're in */
    uint8_t  hdepth;
-    uint16_t tot_elem_cnt;
+    uint32_t tot_elem_cnt;
    uint16_t tot_hash_cnt;
    int16_t  hcnt[SET_HASHTAB_SIZE];


@jeesup0103
하위 node를 가지는 경우에 hcnt[i] = -1 설정을 그대로 유지하고 있네요.

각 hash bucket에 있는 element 개수는 hcnt[i] 값을 확인할 수 있고,
-1이라면 child node 접근하여 tot_elem_cnt 확인한다는 것이죠.

hcnt[i]에서 하위 node에 있는 element 개수를 유지하는 방안도 생각해 볼 수 있는 데,
이 방식을 택하지 않은 이유는 무엇인가요?

@jhpark816
hcnt[hidx] 값을 변경하지 않은 이유는 해당 버킷의 상태를 바로 확인할 수 있도록 유지하기 위해서입니다.
DFS 탐색 과정에서 hcnt[hidx]를 통해 해당 버킷이 추가 노드인지 또는 element chain인지 빠르게 판단할 수 있습니다.

그리고, hcnt[hidx] 값도 하위 노드의 element 개수로 변경한다면, 이는 tot_elem_cnt와 중복 정보를 저장하는 형태가 됩니다.
결국, node->hcnt[hidx]와 node->htab[hidx]->tot_elem_cnt는 같은 값을 가지게 되기 때문에 tot_elem_cnt만 변경하는 것이 더 효율적이라고 생각했습니다.

jhpark816 · 2025-01-10T01:30:20Z

@namsic 추가 리뷰 진행해 주세요.

namsic · 2025-01-13T08:37:10Z

engines/default/coll_set.c

-    node->tot_elem_cnt += 1;
+    node = info->root;
+    while (node != NULL) {
+        node->tot_elem_cnt += 1;


do_set_node_link(), do_set_node_unlink(), do_set_elem_link() 3개 함수는 수정하는데,
do_set_elem_unlink()는 기존 구현을 그대로 사용하고 있습니다.

do_set_elem_traverse_XXX()에서 처리하기 때문인가요?

기존 구현에서는 XXX_link()와 XXX_unlink()에서 tot_elem_cnt 값을 변경하고,
traverse 함수에서는 비교 연산만 수행했습니다.
기존처럼 반대 이름을 갖는 함수의 동작이 서로 짝을 이루도록 구현하기는 어려운가요?

네. 해당 노드의 부모 노드들의 tot_elem_cnt 값은 do_set_elem_traverse_XXX()에서 처리 됩니다.

어렵지 않을 것 같습니다.
do_set_elem_unlink() 에서 do_set_elem_link()와 같이 해시값을 기준으로 while문을 반복해서 부모 노드들의 tot_elem_cnt 값을 업데이트 할 수 있습니다.
그러면 하나의 element를 삭제 할때마다 부모 노드들을 업데이트 해야하는 비용은 발생합니다. (현재 PR에서는 노드당 한번 업데이트)

while (node != NULL) { node->tot_elem_cnt -= 1; hidx = SET_GET_HASHIDX(elem->hval, node->hdepth); if (node->hcnt[hidx] >= 0) break; node = node->htab[hidx]; }

jhpark816

리뷰 완료

jhpark816 · 2025-01-16T08:19:24Z

engines/default/item_base.h

@@ -236,7 +236,7 @@ typedef struct _set_hash_node {
    uint16_t refcount;
    uint8_t  slabs_clsid;         /* which slab class we're in */
    uint8_t  hdepth;
-    uint16_t tot_elem_cnt;
+    uint32_t tot_elem_cnt;
    uint16_t tot_hash_cnt;


unit32_t tot_elem_cnt 는 child node에 있는 elements 수를 포함하는 것으로 변경한다면,
uint16_t tot_hash_cnt 는 제거하는 것이 나을 것 같습니다. 검토 후 조치 바랍니다.

tot_hash_cnt가 이용되는 2가지 경우가 있습니다.

첫째, 아래 경우에서 tot_elem_cnt == 0이면 tot_hash_cnt == 0임을 알 수 있으므로
tot_hash_cnt == 0 조건을 제거해도 됩니다.

if (info->root->tot_hash_cnt == 0 && info->root->tot_elem_cnt == 0) { do_set_node_unlink(info, NULL, 0); }

둘쨰, 아래 경우에서 tot_hash_cnt 필드를 제거하는 대신에
필요하면 get_child_node_count(node) 함수를 두어 계산하게 처리할 수 있습니다.

if (child_node->tot_hash_cnt == 0 && child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2)) { do_set_node_unlink(info, node, hidx); } => if (child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2) && get_child_node_count(node) == 0) { do_set_node_unlink(info, node, hidx); }

그리고 tot_elem_cnt 필드는 그 의미가 변경된 상태이므로,
필드명을 다르게 변경할 필요가 있나요?

네 tot_hash_cnt 제거해도 무방해 보입니다.
다만 두번째 경우에서 실제 child_node의 개수가 필요한 것이 아니기 때문에 is_leaf_node() 와 같은 함수는 어떨까 싶습니다.

구현 예시 if (child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2) && is_leaf_node(child_node)) { do_set_node_unlink(info, node, hidx); } ... bool is_leaf_node(set_hash_node *node) { for (int hidx = 0; hidx < SET_HASHTAB_SIZE; hidx++) { if (node->hcnt[hidx] == -1) return false; } return true; }

tot_elem_cnt 필드명을 변경하는 것이 좋을 것 같습니다. Map 의 tot_elem_cnt 변수와 헷갈릴 여지가 있을 것 같습니다.

변경 예시)

subtree_elem_cnt

subnode_elem_cnt

node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2) 상태이면, child node 또한 없다는 것이 보장되지 않는 지를 확인해 주세요.

tot_elem_cnt 필드명은 일단 그대로 사용합시다.

if (child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2)) 조건만 있어도 가능해 보입니다.

parent_node->tot_elem_cnt >= child_node->tot_elem_cnt 가 보장됩니다.

delete, get delete 명령어에서 DFS 순서상 child_node에서 먼저 node_unlink()가 일어납니다.

결국 부모 노드에 자식 노드가 남아 있다는 것은 해당 자식 노드가 충분히 큰 상태입니다. 그렇지 않다면 unlink().

jhpark816 · 2025-01-16T08:22:57Z

engines/default/coll_set.c

@@ -400,6 +404,7 @@ static ENGINE_ERROR_CODE do_set_elem_traverse_delete(set_meta_info *info, set_ha
                child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2)) {
                do_set_node_unlink(info, node, hidx);
            }
+            node->tot_elem_cnt--;


아래 형태로 변경하시죠.
tot_elem_cnt 변경 스타일을 동일하게 맞추고자 합니다.

tot_elem_cnt -= 1;

engines/default/coll_set.c

jhpark816 · 2025-01-17T06:39:50Z

@jeesup0103
논의는 완료된 것 같으니, 수정해 주세요.

jeesup0103 · 2025-01-17T06:48:42Z

현재 주석 처리되어 있는 do_set_elem_traverse_fast(), do_set_elem_delete_fast()함수에서 node->tot_hash_cnt를 사용하고 있습니다.
해당 부분은 고치지 않아도 되는건가요?

#ifdef SET_DELETE_NO_MERGE
static uint32_t do_set_elem_traverse_fast(set_meta_info *info,
                                          set_hash_node *node, const uint32_t count)
{
    int hidx;
    int fcnt = 0;

    /* node has child node */
    if (node->tot_hash_cnt > 0) {
        for (hidx = 0; hidx < SET_HASHTAB_SIZE; hidx++) {

jhpark816 · 2025-01-17T07:00:11Z

do_set_elem_xxxx_fast() 함수들도 가능하다면, 정확하게 수정해 두면 좋겠습니다.
do_set_elem_xxxx_fast() 함수들은 현재 사용 중인 함수들과는 어떻게 다른가요?
이 함수들에서는 tot_hash_cnt 필드가 반드시 필요한가요?

jeesup0103 · 2025-01-17T08:15:08Z

do_set_elem_XXX_fast() 함수는 node_unlink 작업을 수행하지 않고 최소 count개 삭제 하는 함수입니다.

먼저 hash table을 찾아간 다음, 해당 hash table에 들어있는 모든 element들을 삭제합니다.

그 후 삭제한 element가 count개 이상이면 삭제한 개수를 return 합니다.
그렇지 않다면 다음 hash table에 가서 모든 element를 삭제합니다.

문제 상황은 DFS 순서를 작은 인덱스부터 찾아가도록 변경하면 hash table 단위로 element를 삭제 할 수 없습니다. (순서 변경하면 element chain 단위로 삭제 가능)

그래서 다음과 같이 변경하면 될 것 같습니다.

DFS 순서 유지
tot_hash_cnt 삭제를 위해 함수 내부에 child_node를 가지는지 여부를 확인하는 로직 추가
- for 문을 통해 hcnt[hidx]==-1 이 존재하는지 확인
tot_elem_cnt 현재 PR의 의미와 맞게 변경

다만 더 이상 사용하지 않는 기능이라면 지우는 것이 좋을 것 같습니다.

jeesup0103 · 2025-01-17T08:26:27Z

do_set_elem_XXX_fast() 이슈 제외하고 먼저 수정했습니다.

jhpark816 · 2025-01-17T10:04:36Z

@namsic 최근에 수정된 사항이 있어 재리뷰 요청했습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-01-17T09:19:32Z

engines/default/coll_set.c

@@ -219,8 +218,6 @@ static void do_set_node_link(set_meta_info *info,

        par_node->htab[par_hidx] = node;
        par_node->hcnt[par_hidx] = -1; /* child hash node */
-        par_node->tot_elem_cnt -= num_found;
-        par_node->tot_hash_cnt += 1;


수정하는 김에, num_found, num_elems 변수를 모두 제거해도 될 것 같습니다.

while (par_node->htab[par_hidx] != NULL) { set_elem_item *elem = par_node->htab[par_hidx]; par_node->htab[par_hidx] = elem->next; int hidx = SET_GET_HASHIDX(elem->hval, node->hdepth); elem->next = node->htab[hidx]; node->htab[hidx] = elem; node->hcnt[hidx] += 1; node->tot_elem_cnt += 1; } assert(node->tot_elem_cnt == par_node->hcnt[par_hidx]); par_node->htab[par_hidx] = node; par_node->hcnt[par_hidx] = -1; /* child hash node */

jhpark816 · 2025-01-17T09:28:18Z

engines/default/coll_set.c

@@ -263,13 +258,10 @@ static void do_set_node_unlink(set_meta_info *info,
                assert(node->hcnt[hidx] == 0);
            }
        }
-        assert(fcnt == node->tot_elem_cnt);


여기 assert 문은 그대로 둡시다.

jhpark816 · 2025-01-17T09:48:23Z

engines/default/coll_set.c

+        if (node->hcnt[hidx] >= 0)
+            break;
+        node = node->htab[hidx];
+    }


아래 코드와 같이 현재 노드에서 tot_elem_cnt 증가시키는 코드와
삼위 노드들에서 tot_elem_cnt 증가시키는 코드가 구분되면 좋겠습니다.
do_set_elem_unlink()와의 차이가 잘 드러나게 하려는 의도입니다.

elem->next = node->htab[hidx]; node->htab[hidx] = elem; node->hcnt[hidx] += 1; node->tot_elem_cnt += 1; set_hash_node *par_node = info->root; while (par_node != node) { par_node->tot_elem_cnt += 1; hidx = SET_GET_HASHIDX(elem->hval, par_node->hdepth); assert(par_node->hcnt[hidx] == -1); par_node = par_node->htab[hidx]; } info->ccnt++;

jhpark816 · 2025-01-18T02:15:16Z

engines/default/coll_set.c

@@ -396,10 +395,10 @@ static ENGINE_ERROR_CODE do_set_elem_traverse_delete(set_meta_info *info, set_ha
        set_hash_node *child_node = node->htab[hidx];
        ret = do_set_elem_traverse_delete(info, child_node, hval, val, vlen);
        if (ret == ENGINE_SUCCESS) {
-            if (child_node->tot_hash_cnt == 0 &&
-                child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2)) {
+            if (child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2)) {


child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2) 조건 만으로
child_node가 leaf node인지를 100% 보장할 수 있는 지가 아직 의문인데요.
어떤가요?

조금이라도 확신이 서지 않는다면, 아래와 같이 is_leaf_node() 추가하면 좋겠습니다.

if (child_node->tot_elem_cnt < (SET_MAX_HASHCHAIN_SIZE/2) && is_leaf_node(child_node)) { do_set_node_unlink(info, node, hidx); }

jeesup0103 marked this pull request as ready for review January 8, 2025 06:10

jeesup0103 changed the title ~~INTERNAL: Change tot_elem_cnt to subtree element count in se~~ INTERNAL: Change tot_elem_cnt to subtree element count in set Jan 8, 2025

namsic self-requested a review January 8, 2025 10:33

jhpark816 requested review from ing-eoking and removed request for namsic January 9, 2025 01:11

jeesup0103 force-pushed the internal/set-variable branch from 50d0a1d to dd999d1 Compare January 9, 2025 01:42

jeesup0103 marked this pull request as draft January 9, 2025 02:00

jeesup0103 force-pushed the internal/set-variable branch from dd999d1 to 6b2805e Compare January 9, 2025 02:37

jeesup0103 marked this pull request as ready for review January 9, 2025 02:37

jeesup0103 force-pushed the internal/set-variable branch 2 times, most recently from 8242e3a to 2199b09 Compare January 9, 2025 08:20

ing-eoking approved these changes Jan 9, 2025

View reviewed changes

ing-eoking requested a review from jhpark816 January 9, 2025 08:39

jhpark816 reviewed Jan 9, 2025

View reviewed changes

jhpark816 requested a review from namsic January 10, 2025 01:30

namsic reviewed Jan 13, 2025

View reviewed changes

namsic approved these changes Jan 14, 2025

View reviewed changes

jhpark816 reviewed Jan 16, 2025

View reviewed changes

INTERNAL: Change tot_elem_cnt to subtree element count in set

3782096

jeesup0103 force-pushed the internal/set-variable branch from 2199b09 to 3782096 Compare January 17, 2025 08:25

jhpark816 requested a review from namsic January 17, 2025 10:03

jhpark816 reviewed Jan 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Change tot_elem_cnt to subtree element count in set #817

INTERNAL: Change tot_elem_cnt to subtree element count in set #817

jeesup0103 commented Jan 8, 2025 •

edited

Loading

jhpark816 Jan 9, 2025

jeesup0103 Jan 10, 2025

jhpark816 commented Jan 10, 2025

namsic Jan 13, 2025

jeesup0103 Jan 14, 2025 •

edited

Loading

jhpark816 left a comment

jhpark816 Jan 16, 2025

jeesup0103 Jan 17, 2025 •

edited

Loading

jhpark816 Jan 17, 2025

jeesup0103 Jan 17, 2025

jhpark816 Jan 16, 2025

jhpark816 commented Jan 17, 2025

jeesup0103 commented Jan 17, 2025 •

edited

Loading

jhpark816 commented Jan 17, 2025 •

edited

Loading

jeesup0103 commented Jan 17, 2025

jeesup0103 commented Jan 17, 2025

jhpark816 commented Jan 17, 2025

jhpark816 left a comment

jhpark816 Jan 17, 2025

jhpark816 Jan 17, 2025

jhpark816 Jan 17, 2025

jhpark816 Jan 18, 2025

INTERNAL: Change tot_elem_cnt to subtree element count in set #817

Are you sure you want to change the base?

INTERNAL: Change tot_elem_cnt to subtree element count in set #817

Conversation

jeesup0103 commented Jan 8, 2025 • edited Loading

🔗 Related Issue

⌨️ What I did

세부 구현 내용

추후 PR 내용

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Jan 10, 2025

Choose a reason for hiding this comment

jeesup0103 Jan 14, 2025 • edited Loading

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

jeesup0103 Jan 17, 2025 • 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

jhpark816 commented Jan 17, 2025

jeesup0103 commented Jan 17, 2025 • edited Loading

jhpark816 commented Jan 17, 2025 • edited Loading

jeesup0103 commented Jan 17, 2025

jeesup0103 commented Jan 17, 2025

jhpark816 commented Jan 17, 2025

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

jeesup0103 commented Jan 8, 2025 •

edited

Loading

jeesup0103 Jan 14, 2025 •

edited

Loading

jeesup0103 Jan 17, 2025 •

edited

Loading

jeesup0103 commented Jan 17, 2025 •

edited

Loading

jhpark816 commented Jan 17, 2025 •

edited

Loading