ENHANCE: optimize do_item_replace() by dongwooklee96 · Pull Request #508 · naver/arcus-memcached

dongwooklee96 · 2020-09-08T16:46:54Z

do_item_replace() 함수를 최적화 하였습니다.

기존의 item_unlink(), item_link() 함수를 호출하는 방식에서
replace 함수 이름에 맞게 대체 해주는 방식으로 최적화 하였습니다.

assoc_replace() 함수 생성 및 대체하는 과정에서 일어나는 불필요한 코드 제거
prefix_replace() 함수 생성 및 대체하는 과정에서 일어나는 불필요한 코드 제거
do_item_stat_replace() 함수를 생성 및 stotal 변경 사항을 한번에 적용함으로써 최적화

MinWooJin

리뷰 완료 했습니다.
수정 후 리뷰 진행 하겠습니다.

MinWooJin · 2020-09-08T23:08:34Z

+        /* Cache item replacement does not drop the prefix item even if it's empty.
+         * So, the below do_item_link function always return SUCCESS.
+         */
+        key = item_get_key(new_it);


MEMCACHED_ITEM_LINK macro가 사라지면, 같이 제거할 수 있겠습니다.

MinWooJin · 2020-09-08T23:32:57Z

    return 1;
 }

+void assoc_replace(hash_item *old_it, hash_item *new_it)


현재 구현은 assoc_delete와 assoc_insert를 순차적으로 호출한것과 같은 코드입니다.

아래와 같이 하는건 어떤가요?

old_it의 before 방향에 있는 item 검색

존재하지 않는다면

대략 아래와 같은 로직 수행

new_it->next = old_it->next

hashtable[bucket] = new_it

존재한다면

대략 아래와 같은 로직 수행

prev->next = new_it

next_it->next = old_it->next

제안한 로직이 잘 맞지 않고, 오히려 코드가 더 복잡해진다면 comment 해주세요.

리뷰주셔서 감사합니다
리뷰 받은 내용대로 수정해보겠습니다,

2차 리뷰 반영 및 수정 했습니다.

MinWooJin · 2020-09-08T23:40:00Z

+
+        prefix_replace(old_it, new_it, old_stotal, new_stotal);
+
+        do_item_stat_replace(new_it, old_stotal, new_stotal);


define 한 함수 argument와 맞지 않게 호출하고 있습니다.

함수 정의 부분을 올바르게 바꿔주었습니다

MinWooJin

수정 부분에 대한 리뷰 완료 했습니다.
세부 코드 수정 이후 한번 더 보겠습니다.

MinWooJin · 2020-09-09T06:13:48Z

+         * due to possible tail-optimization by the compiler
+         */
+        MEMCACHED_ASSOC_DELETE(key, old_it->nkey, assocp->hash_items);
+        if (*before != old_it) {


hashitem_before()를 통해 old_it을 찾았는데, *before != old_it 일 경우가 있나요?

MinWooJin · 2020-09-09T06:15:36Z

+    }
+    /* Note:  we never actually get here.  the callers don't delete things
+       they can't find. */
+    assert(*before != 0);


여기 assert가 필요 한가요?

MinWooJin · 2020-09-09T06:17:18Z

+{
+    hash_item **before = _hashitem_before(item_get_key(old_it), old_it->nkey, old_it->khash);
+
+    if (*before) {


*before가 NULL일 수 있나요?

MinWooJin · 2020-09-09T06:18:09Z

+        }
+        /* Cache item replacement does not drop the prefix item even if it's empty.
+         * So, the below do_item_link function always return SUCCESS.
+         */


더 이상 do_item_link 함수를 사용하지 않습니다. 주석을 변경하거나, 제거해야 할 것 같습니다.

이부분은 반영되지 않았습니다. 반영할 필요가 없다면 코멘트 남겨주세요.

dongwooklee96 · 2020-09-09T07:41:59Z

수정 부분에 대한 리뷰 완료 했습니다.
세부 코드 수정 이후 한번 더 보겠습니다.

리뷰 반영 하여 코드 수정 완료 하였습니다.

MinWooJin

리뷰 완료 했습니다.

MinWooJin · 2020-09-09T23:04:56Z

+    (old_it)->h_next = NULL;
+
+    MEMCACHED_ASSOC_INSERT(item_get_key(new_it), new_it->nkey, assocp->hash_items);
+    return;


return은 필요 없습니다.

MinWooJin · 2020-09-09T23:05:53Z

+        }
+        /* Cache item replacement does not drop the prefix item even if it's empty.
+         * So, the below do_item_link function always return SUCCESS.
+         */


이부분은 반영되지 않았습니다. 반영할 필요가 없다면 코멘트 남겨주세요.

MinWooJin · 2020-09-09T23:21:25Z

+    /* The DTrace probe cannot be triggered as the last instruction
+     * due to possible tail-optimization by the compiler
+     */
+    MEMCACHED_ASSOC_DELETE(key, old_it->nkey, assocp->hash_items);


MEMCACHED_ASSOC_REPLACE 를 정의하고 dtrace probe 작성 까지 해줄지,
그대로 DELETE와 INSERT를 호출하는 것으로 둘지에 대한 고민이 있네요.
@jhpark816
같이 검토해주시면 좋을 것 같습니다.

MEMCACHED_ASSOC_REPLACE 매크로를 새로 정의하지 말고,
기존대로 MEMCACHED_ASSOC_DELETE와 MEMCACHED_ASSOC_INSERT 매크로를 사용합시다.

현재는 unlink & link 작업으로 구성된 replace 작업의 처리 방식을 최적화하는 작업입니다.
Replace 개념을 완전하게 구현하려면,

persistence, replication 등에서도 unlink and link 방식이 아니라 replace 방식으로 변경해야 합니다.

그 작업을 수행할 시에 매크로 등을 함께 변경하면 됩니다.

jhpark816

review 완료

jhpark816 · 2020-09-10T00:28:51Z

+    /* The DTrace probe cannot be triggered as the last instruction
+     * due to possible tail-optimization by the compiler
+     */
+    MEMCACHED_ASSOC_DELETE(key, old_it->nkey, assocp->hash_items);


MEMCACHED_ASSOC_REPLACE 매크로를 새로 정의하지 말고,
기존대로 MEMCACHED_ASSOC_DELETE와 MEMCACHED_ASSOC_INSERT 매크로를 사용합시다.

현재는 unlink & link 작업으로 구성된 replace 작업의 처리 방식을 최적화하는 작업입니다.
Replace 개념을 완전하게 구현하려면,

persistence, replication 등에서도 unlink and link 방식이 아니라 replace 방식으로 변경해야 합니다.

그 작업을 수행할 시에 매크로 등을 함께 변경하면 됩니다.

Sign in to view

+
+        /* replace the item from hash table */
+        assoc_replace(old_it, new_it);
+


jhpark816 · 2020-09-10T00:52:57Z

+
+        prefix_replace(old_it, new_it, old_stotal, new_stotal);
+
+        do_item_stat_replace(new_it, old_stotal, new_stotal);


do_item_stat_replace() 함수 안에서

prefix에 있는 통계 정보까지도 함께 갱신하도록 하시죠.

prefix_replace()라는 함수를 별도로 두지 말고,
기존에 있던 prefix_bytes_incr() 함수와 prefix_bytes_decr() 함수를 이용하도록 하시죠.

prefix_replace() 함수를 두지 않으려는 이유는
prefix의 item bytes 정보를 갱신하는 함수가 기존에 존재하므로, 그 함수를 이용하려는 것입니다.

prefix bytes 갱신 함수는 prefix 모듈의 역할이므로, prefix 모듈에 맡겨두는 것이 좋습니다.

참고로, 현재 다른 PR에서 prefix bytes 갱신 코드를 수정하고 있습니다.
그 PR에 영향을 주지 않으려면, 기존 interface에 있는 함수를 이용해야 합니다.

prefix 모듈의 담당자가 interface가 적합하지 않다면, 나중에 수정하면 됩니다.

참고로, assoc_replace() 함수는 기존에 그 기능이 없었기 때문에 추가한 것입니다.

리뷰 주셔서 감사합니다,

prefix_replace()라는 함수를 별도로 두지 말고,
기존에 있던 prefix_bytes_incr() 함수와 prefix_bytes_decr() 함수를 이용하도록 하시죠.

prefix_bytes_incr(), prefix_bytes_decr() 함수에 아래와 같은 assert 문이 있어
item 타입이 collection element 일때 만 호출할 수 있게 되어있어서 함수들을 이용할 수 없는 것 같은데 맞나요?

void prefix_bytes_incr(prefix_t *pt, ENGINE_ITEM_TYPE item_type, const uint32_t bytes) { /* It's called when a collection element is inserted */ assert(item_type > ITEM_TYPE_KV && item_type < ITEM_TYPE_MAX); ...

그리고 prefix_bytes_incr(), prefix_bytes_decr() 함수에 위와 같은 assert가 필요한 배경이 궁금합니다.

@dongwooklee96
알려주어서 감사합니다.
그럼, assert() 문을 수정하는 것이 좋겠습니다.

assert(item_type < ITEM_TYPE_MAX);

참고 사항으로,

예전에 collection type에 한정하여 사용하던 함수인 데,
이번에 replace 최적화 작업에서 이용할 수 있도록 확장하는 것이 좋겠습니다.

prefix_replace() 함수를 추가하는 것보다 기존 함수의 확장이 낫다고 생각합니다.

아 그렇군요 감사합니다! 리뷰 주신 내용대로 수정했습니다.

dongwooklee96 · 2020-09-10T11:07:40Z

리뷰 주신 내용대로 수정했습니다!

jhpark816

review 완료.
최종 리뷰가 될 것 같습니다.

jhpark816 · 2020-09-11T03:49:04Z

+        size_t old_stotal = ITEM_stotal(old_it);
+        size_t new_stotal = ITEM_stotal(new_it);
+
+        do_item_stat_replace(old_it, new_it, old_stotal, new_stotal);


여기 코드를 몇 가지만 수정하면 좋겠습니다.

do_item_stat_replace()에 old_it, new_it 인자 2개만 전달합시다.

old_stotal과 new_stotal은 함수 내부에서 구하는 것이 좋겠습니다.

new_it->pfxptr 설정을 do_item_stat_replace() 호출 이전에 하면 좋겠습니다

do_item_stat_replace()는 통계 정보만 replace하면 좋겠습니다.

new_it->pfxptr 설정한 후, ITEM_INTERNAL iflag 설정을 추가하여야 합니다.

응용에서 생성한 item이 아니라 arcus 시스템 자체가 어떤 목적으로 생성하는 item이 있습니다.
"arcus" 라는 prefix를 가지고 있는 item입니다

이러한 item을 internal item이라 하고 ITEM_INTERNAL iflag로 설정해 두고 있습니다.

따라서, old item의 iflag에 ITEM_INTERNAL가 설정되어 있다면, new item에도 그대로 설정하면 됩니다.

추가적으로 old_stotal과 new_stotal을 do_item_stat_replace() 함수에서 구하도록 안으로 넣고 old_stotal, new_stotal 파라미터를 제거하는 방향으로 변경하는것은 어떨까요?

old_stotal과 new_stotal을 do_item_stat_replace() 함수에서 구하도록 안으로 넣는다는 것이죠 ?
오타가 있어 질문을 다시 하는 것이며, 맞다면 같은 생각힙니다.
do_item_stat_replace()에 old_it, new_it 인자 2개만 전달한다는 것이 old_stotal, new_stotal은 제거하자는 의미입니다.

네 맞습니다! 앗 오타가 있었네요 죄송합니다 .
그럼 그렇게 변경하도록 하겠습니다

dongwooklee96 · 2020-09-11T05:10:05Z

최종 리뷰 반영하였습니다!

jhpark816 · 2020-09-11T07:38:06Z

@dongwooklee96

아래 사항이 아직 반영되지 않은 것 같습니다. 확인 바랍니다.

new_it->pfxptr 설정을 do_item_stat_replace() 호출 이전에 하면 좋겠습니다
- do_item_stat_replace()는 통계 정보만 replace하면 좋겠습니다.
- 즉, do_item_replace()에서 do_item_stat_replace() 호출 직전에 설정하면 좋겠습니다.
new_it->pfxptr 설정한 후, new_it의 ITEM_INTERNAL iflag 설정도 추가하여야 합니다.
- 응용에서 생성한 item이 아니라 arcus 시스템 자체가 어떤 목적으로 생성하는 item이 있습니다.
  "arcus" 라는 prefix를 가지고 있는 item입니다
- 이러한 item을 internal item이라 하고 ITEM_INTERNAL iflag로 설정해 두고 있습니다.
- 따라서, old item의 iflag에 ITEM_INTERNAL가 설정되어 있다면, new item에도 그대로 설정하면 됩니다.
- do_item_link() 함수를 참고 바랍니다.

jhpark816 · 2020-09-11T07:39:53Z


+static inline void do_item_stat_replace(hash_item *old_it, hash_item *new_it)
+{
+    LOCK_STATS();


한가지만 더 얘기하면, LOCK_STATS()은 if (new_stotal != old_stotal) 직전에 있으면 될 것 같습니다.

앗 죄송합니다! 리뷰 반영안된 부분이 있는지 몰랐네요. 좀 더 신경쓰겠습니다.

Previously, it was a function to support only collection types, but we changed the assert condition to support all item types.

update prefix info and item stats info when calling do_item_replace() function.

jhpark816

review 완료.

jhpark816 · 2020-09-11T09:01:41Z

+
+    /* if old_it->iflag has ITEM_INTERNAL */
+    if (old_it->iflag & ITEM_INTERNAL) {
+        new_it->iflag = ITEM_INTERNAL;


특정 비트를 설정하는 작업이므로, 아래와 같이 수정되어야 합니다.

new_it->iflag |= ITEM_INTERNAL;

알려주셔서 감사합니다!

jhpark816 · 2020-09-11T09:26:38Z

@dongwooklee96
수고가 많았으며, contribution 감사합니다.

dongwooklee96 · 2020-09-11T09:52:05Z

리뷰해주셔서 감사합니다! 덕분에 많이 배울 수 있었습니다. @jhpark816 @MinWooJin @minkikim89

MinWooJin suggested changes Sep 8, 2020

View reviewed changes

dongwooklee96 force-pushed the issue#502_v2 branch from 474f39c to f7f14da Compare September 9, 2020 00:48

dongwooklee96 requested a review from MinWooJin September 9, 2020 00:53

dongwooklee96 force-pushed the issue#502_v2 branch 2 times, most recently from f88889c to 621bf08 Compare September 9, 2020 04:25

MinWooJin suggested changes Sep 9, 2020

View reviewed changes

dongwooklee96 force-pushed the issue#502_v2 branch from 621bf08 to 8af2b0b Compare September 9, 2020 06:53

dongwooklee96 force-pushed the issue#502_v2 branch from 8af2b0b to 1e85c13 Compare September 9, 2020 07:51

MinWooJin reviewed Sep 9, 2020

View reviewed changes

MinWooJin approved these changes Sep 9, 2020

View reviewed changes

MinWooJin requested review from MinWooJin and jhpark816 and removed request for MinWooJin September 9, 2020 23:30

jhpark816 reviewed Sep 10, 2020

View reviewed changes

dongwooklee96 force-pushed the issue#502_v2 branch from 1e85c13 to f8e7ba8 Compare September 10, 2020 10:28

dongwooklee96 requested a review from jhpark816 September 11, 2020 03:02

jhpark816 reviewed Sep 11, 2020

View reviewed changes

dongwooklee96 force-pushed the issue#502_v2 branch from f8e7ba8 to 4e9b345 Compare September 11, 2020 05:05

dongwooklee96 requested a review from jhpark816 September 11, 2020 05:47

jhpark816 reviewed Sep 11, 2020

View reviewed changes

dongwooklee96 added 3 commits September 11, 2020 17:46

CLEANUP: change assert condition to support all item types.

ff6556d

Previously, it was a function to support only collection types, but we changed the assert condition to support all item types.

FEATURE: add assoc_replace function.

0e489e2

FEATURE: add do_item_stat_replace function.

8a9de00

update prefix info and item stats info when calling do_item_replace() function.

dongwooklee96 force-pushed the issue#502_v2 branch from 4e9b345 to 2e2eb2b Compare September 11, 2020 08:48

jhpark816 reviewed Sep 11, 2020

View reviewed changes

ENHANCE: modify do_item_replace function for optimization.

627c70f

dongwooklee96 force-pushed the issue#502_v2 branch from 2e2eb2b to 627c70f Compare September 11, 2020 09:14

dongwooklee96 requested a review from jhpark816 September 11, 2020 09:25

jhpark816 merged commit e18707d into naver:develop Sep 11, 2020

jhpark816 mentioned this pull request Sep 11, 2020

item replace 연산의 최적화 #502

Closed


		prefix_replace(old_it, new_it, old_stotal, new_stotal);

		do_item_stat_replace(new_it, old_stotal, new_stotal);


		/* replace the item from hash table */
		assoc_replace(old_it, new_it);

Uh oh!

Conversation

dongwooklee96 commented Sep 8, 2020

Uh oh!

MinWooJin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MinWooJin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwooklee96 commented Sep 9, 2020

Uh oh!

MinWooJin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

jhpark816 Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwooklee96 Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwooklee96 Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwooklee96 commented Sep 10, 2020

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongwooklee96 Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 Sep 10, 2020 •

edited

Loading

dongwooklee96 Sep 10, 2020 •

edited

Loading

dongwooklee96 Sep 10, 2020 •

edited

Loading

dongwooklee96 Sep 11, 2020 •

edited

Loading

dongwooklee96 Sep 11, 2020 •

edited

Loading

dongwooklee96 commented Sep 11, 2020 •

edited

Loading