INTERNAL: Refactor the memcached_io_read function #341

ing-eoking · 2025-01-21T03:24:46Z

🔗 Related Issue

INTERNAL: Fix an overwrite issue for the big key #338

⌨️ What I did

별도의 PR로 두는 편이 더 적합할 것 같아 따로 작업했습니다.
memcached_io_read 로직을 리팩토링했습니다.
- buffer 인자가 NULL인 경우, read buffer에서 length만큼의 데이터를 버리도록 수정했습니다.
- memcached_io_read는 내부 함수이기에 조심하면 문제가 없을 것으로 생각되지만, 혹시 모를 경우에 대비해 nread가 NULL인 경우에 unused_nread를 활용하도록 처리했습니다.
- 기존 로직에서는 length가 1인 경우와 1을 초과한 경우를 구분하여 처리했는데, memcpy 부분을 제외하고 성능 상 큰 문제는 없을 것으로 판단하여 PR과 같이 변경했습니다.

jhpark816 · 2025-01-21T03:49:36Z

memcached_safe_read() 함수가 아닌 memcached_io_read() 함수를 수정한 것인가요?

ing-eoking · 2025-01-21T04:34:53Z

memcached_safe_read() 함수가 아닌 memcached_io_read() 함수를 수정한 것인가요?

네. io_read에서 수정을 하면, 별도의 buffer가 필요하지 않아 해당 함수를 수정했습니다.

현재 CI test(ubuntu)를 실패하여 잠시 수정할 때까지 draft로 변경하겠습니다.

ing-eoking · 2025-01-21T07:10:02Z

@jhpark816

수정되었습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-01-21T10:56:41Z

libmemcached/io.cc

@@ -538,8 +538,12 @@ memcached_return_t memcached_io_read(memcached_server_write_instance_st ptr,
    return MEMCACHED_CONNECTION_FAILURE;
  }

+  ssize_t unused_nread;
+  if (nread == NULL) nread= &unused_nread;


기존 코드에서
nread == NULL 검사 없이 *nread= xxx; 코드가 존재했다는 것은
nread가 NULL인 경우는 없다는 것을 의미합니다.

따라서, memcached_io_read() 호출하는 쪽의 변경이 없는 상황에서
nread == NULL 조건을 검사할 필요는 없어 보입니다.

libmemcached/io.cc

jhpark816

리뷰 완료

libmemcached/io.cc

jhpark816

리뷰 완료

jhpark816 · 2025-01-22T01:54:59Z

libmemcached/io.cc

@@ -539,6 +539,7 @@ memcached_return_t memcached_io_read(memcached_server_write_instance_st ptr,
  }

  char *buffer_ptr= static_cast<char *>(buffer);
+  *nread= 0;


0으로 설정할 필요가 없습니다.

jhpark816 · 2025-01-22T01:56:14Z

libmemcached/io.cc

+      if (buffer_ptr) {
+        memcpy(buffer_ptr, ptr->read_ptr, difference);
+        buffer_ptr+= difference;
+      }

      length-= difference;


length 변경은 아래로 내려주세요.

예시 코드를 참고 바랍니다.

수정되었습니다.

어디서 수정되었나요?

jhpark816 · 2025-01-22T01:56:35Z

libmemcached/io.cc


-    offset+= (size_t) nread;


이렇게 수정해야 하는 이유는 무엇인가요?

@jhpark816
현재 변경 방식에서 buffer(data) 인자가 NULL인 경우, nread 값은 0이게 됩니다.
기존 safe_read는 nread 결과 값으로부터 offset을 계산했으므로 buffer가 NULL인 상황에서
offset 값이 증가하지 않아 무한루프에 빠지게 됩니다.

그리고 이미 memcached_io_read에서 while 문을 통해 지정된 길이만큼 모두 읽는 것이 보장되므로, memcached_safe_read에서는 while (offset < size) 문을 둘 필요가 없어 제거했습니다.

@ing-eoking

현재 변경 방식에서 buffer(data) 인자가 NULL인 경우, nread 값은 0이게 됩니다.

어떤 코드 때문에 nread 값이 0이 되나요?

수정 내용이 반영되지 않아 다시 올렸습니다.

어떤 코드 때문에 nread 값이 0이 되나요?

*nread = (ssize_t)(buffer_ptr - (char*)buffer); 에 의하여 0이 됩니다.

nread 정보는 정확히 전달되어야 합니다.
그래서, 앞서 PR에서 nread에다 매번 읽은 크기를 더한 것이군요.

nread에다 계속 더하는 것 보다,
현재 length 값을 감소시키고 있으니,

request = length; 값을 가지는 request 변수를 두고,

나중에 *nread = request - length; 형태로 설정하면 될 것 같습니다.

변수 명이 조금 어색하네요.
length를 그대로 두고, 잔여크기 의미의 변수를 두어 read할 때마다 감소시키는 것이 나을 것 같기도 합니다.

이에 대해 의견 주세요.

변수 명이 조금 어색하네요.
length를 그대로 두고, 잔여크기 의미의 변수를 두어 read할 때마다 감소시키는 것이 나을 것 같기도 합니다.

저는 remain 또는 left라는 변수를 사용하여 위 방식으로 처리하는 것이 자연스러울 것 같습니다.

remains 변수를 사용하시죠.

remain으로 먼저 바꿔서 remains로 재변경하여 다시 push 했습니다.

nread 값을 변경했으니,
memcached_safe_read() 변경은 없어도 될 것 같습니다.

buffer가 NULL일 경우 잘못된 계산이 발생하므로 일부는 수정하겠습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-01-22T05:54:07Z

libmemcached/io.cc

+      if (buffer_ptr) {
+        memcpy(buffer_ptr, ptr->read_ptr, difference);
+        buffer_ptr+= difference;
+      }



여기 empty line은 제거해 주시고요.

jhpark816 · 2025-01-22T05:54:28Z

libmemcached/io.cc

+      if (buffer_ptr) {
+        *buffer_ptr= *ptr->read_ptr;
+        buffer_ptr++;
+      }


아래에서 remains--; 추가해야 합니다.

jhpark816 · 2025-01-22T05:54:46Z

libmemcached/io.cc

      ptr->read_ptr++;
      ptr->read_buffer_length--;
-      buffer_ptr++;
      break;


reamins--; 추가되면, break 문은 없어도 됩니다.

jhpark816 · 2025-01-22T05:55:21Z

libmemcached/io.cc

      ptr->read_ptr+= difference;
      ptr->read_buffer_length-= difference;
-      buffer_ptr+= difference;
+      remains-= difference;


remains > 1 경우가 remains == 1인 경우로 구분되어 있는 데,
remains > 1 경우의 코드만 있어도 될 것 같습니다.

remains > 1인 경우와 remains == 1을 구분하고 있는 이유가 remains > 1인 경우 memcpy를 쓰고, remains == 1인 경우 등호 연산자(=)를 쓰려는 의도로 보입니다.

모두 memcpy로 통일할까요?

우선 해당 부분은 그대로 두었습니다.

나머지 수정되엇습니다.

질문입니다.
remains == 1인 경우가 많을 수 있나요?
많지 않다면, 아래 코드만으로 충분할 것 같습니다.

size_t difference= (remains > ptr->read_buffer_length) ? ptr->read_buffer_length : remains; if (buffer_ptr) { memcpy(buffer_ptr, ptr->read_ptr, difference); buffer_ptr+= difference; } ptr->read_ptr+= difference; ptr->read_buffer_length-= difference; remains-= difference;

내부 코드에서 length를 1로 두고 호출하는 경우가 있습니다.

memcached_io_readline:

read_buffer가 비어 있을 경우, memcached_io의 length 인자에 1을 넘겨 read_buffer를 채웁니다.

fetch_value_header:

컬렉션 요소의 bkey, eflag 등을 읽을 때, 하나씩 읽어들이며, space, \r, \n이 나올 때까지 반복합니다.

읽어야 할 길이는 모르고,
단지 특정 문자(\n)가 나올 떄까지 읽기 위함이군요.

그러면, remains == 1 인 경우의 코드를 그대로 유지하시죠.

jhpark816 · 2025-01-22T05:56:10Z

local에서 한번 수정할 때 전체를 다 수정해 주시고,
문제 없는 지를 확인한 후에 github에 올려주세요.

jhpark816

리뷰 완료

ing-eoking force-pushed the resp branch from 2b95f1f to cc873d7 Compare January 21, 2025 06:33

jhpark816 reviewed Jan 21, 2025

View reviewed changes

ing-eoking force-pushed the resp branch 4 times, most recently from 6405f01 to fbe9fc1 Compare January 22, 2025 01:10

jhpark816 reviewed Jan 22, 2025

View reviewed changes

libmemcached/io.cc Show resolved Hide resolved

ing-eoking force-pushed the resp branch from fbe9fc1 to 9b3f24a Compare January 22, 2025 01:46

jhpark816 reviewed Jan 22, 2025

View reviewed changes

ing-eoking force-pushed the resp branch 3 times, most recently from 7047c10 to 9dc7b0d Compare January 22, 2025 05:49

jhpark816 reviewed Jan 22, 2025

View reviewed changes

ing-eoking force-pushed the resp branch from 9dc7b0d to 0ce08d1 Compare January 22, 2025 07:00

INTERNAL: Refactor the memcached_io_read function

8ca811d

ing-eoking force-pushed the resp branch from 0ce08d1 to 8ca811d Compare January 22, 2025 07:02

jhpark816 reviewed Jan 22, 2025

View reviewed changes

jhpark816 approved these changes Jan 22, 2025

View reviewed changes

jhpark816 merged commit 6804888 into naver:develop Jan 22, 2025
1 check passed

ing-eoking deleted the resp branch January 22, 2025 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Refactor the memcached_io_read function #341

INTERNAL: Refactor the memcached_io_read function #341

ing-eoking commented Jan 21, 2025

jhpark816 commented Jan 21, 2025

ing-eoking commented Jan 21, 2025

ing-eoking commented Jan 21, 2025

jhpark816 left a comment

jhpark816 Jan 21, 2025

jhpark816 left a comment

jhpark816 left a comment

jhpark816 Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025

jhpark816 Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025 •

edited

Loading

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025 •

edited

Loading

jhpark816 Jan 22, 2025 •

edited

Loading

ing-eoking Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025

jhpark816 left a comment

jhpark816 Jan 22, 2025

jhpark816 Jan 22, 2025

jhpark816 Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025

ing-eoking Jan 22, 2025

jhpark816 Jan 22, 2025

ing-eoking Jan 22, 2025

jhpark816 Jan 22, 2025

jhpark816 commented Jan 22, 2025

jhpark816 left a comment

INTERNAL: Refactor the memcached_io_read function #341

INTERNAL: Refactor the memcached_io_read function #341

Conversation

ing-eoking commented Jan 21, 2025

🔗 Related Issue

⌨️ What I did

jhpark816 commented Jan 21, 2025

ing-eoking commented Jan 21, 2025

ing-eoking commented Jan 21, 2025

jhpark816 left a 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

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

ing-eoking Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

jhpark816 Jan 22, 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

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

jhpark816 commented Jan 22, 2025

jhpark816 left a comment

Choose a reason for hiding this comment

ing-eoking Jan 22, 2025 •

edited

Loading

ing-eoking Jan 22, 2025 •

edited

Loading

jhpark816 Jan 22, 2025 •

edited

Loading