INTERNAL: Refactor lqdetect show

[cheesecrust]

🔗 Related Issue

• https://github.com/jam2in/arcus-works/issues/554

⌨️ What I did

• 기존 <value, length> 구조의 배열을 리턴하는 로직에서 간단하게 filed_t 하나만을 반환하는 로직으로 변경하였습니다.
• 기존의 char * 를 구조체에 담는 방식에서 실제 출력 하는 문자열을 이어붙여 만들어서 리턴하도록 함수를 변경하였습니다.
• 문자열을 이어붙이기 위한 공간을 할당하기 전에 이어붙일 문자열의 길이를 측정하여 이에 맞는 공간 만큼만 할당하도록 합니다.
  • 이때 길이를 측정할 때 header 는 기존과 같이 최대 길이를 32 라고 두고 초기화 하였습니다.
  • body 는 lqdetect 에 total_offset 을 선언하여 buffer 의 offset 이 증가할때 같이 증가하여 이를 활용했습니다.
• 실패 할 경우의 message 는 SERVER ERROR out of memory writing show response. 를 출력하도록 하였습니다.
• lq->result 를 삭제함으로서 lq_release 삭제했습니다.

[namsic]

따라서 함수 호출 시 가질 수 있는 최장 길이 만큼을 메모리에 할당하고 write_and_free 를 사용하였습니다.

상당히 큰 메모리를 할당하고 해제하게 되는데, 생각해 본 다른 구현 방안이 있나요?

[cheesecrust]

따라서 함수 호출 시 가질 수 있는 최장 길이 만큼을 메모리에 할당하고 write_and_free 를 사용하였습니다.

상당히 큰 메모리를 할당하고 해제하게 되는데, 생각해 본 다른 구현 방안이 있나요?

방식을 바꾸어서 먼저 출력할 문자열의 길이를 잰 다음에 그에 맞게 할당하는 방식으로 구현을 바꾸었습니다.

[ing-eoking]

아래와 같은 방식은 어떤가요?

field_t lqdetect_result_get(void)
{
    int hdrlen = 32;
    field_t result = {NULL, 0};

    pthread_mutex_lock(&lqdetect.lock);
    int bytes = hdrlen * LQ_CMD_NUM;
    for (int i = 0; i < LQ_CMD_NUM; i++) {
        bytes += lqdetect.buffer[i].offset;
    }
    if ((result.value = (char*)malloc(bytes)) != NULL) {
        char *nxt = result.value;
        for (int i = 0; i < LQ_CMD_NUM; i++) {
            struct lq_detect_buffer ldb = lqdetect.buffer[i];
            nxt += snprintf(nxt, bytes, "%s : %u\n", command_str[i], ldb.ntotal);
            bytes -= (nxt - result.value);
            if (ldb.ntotal != 0) {
                int length = bytes < ldb.offset ? bytes : ldb.offset;
                nxt += snprintf(nxt, length, "%s", ldb.data);
                bytes -= length;
            }
        }
        result.length = nxt - result.value;
    }
    pthread_mutex_unlock(&lqdetect.lock);

    return result;
}

[cheesecrust]

        nxt += snprintf(nxt, bytes, "%s : %u\n", command_str[i], ldb.ntotal);
        bytes -= (nxt - result.value);
        if (ldb.ntotal != 0) {
            int length = bytes < ldb.offset ? bytes : ldb.offset;
            nxt += snprintf(nxt, length, "%s", ldb.data);
            bytes -= length;

• byte 계산에 있어서 이미 뺀 바이트 값 만큼 연속해서 계속 빼는 오류가 있어 아래와 같이 정정하였습니다.
• snprintf 의 최대 바이트 부분을 ldb.offset + 1 로 수정하였습니다.

       nxt += snprintf(nxt, bytes - (nxt - result.value), "%s : %u\n", command_str[i], ldb.ntotal);
       if (ldb.ntotal != 0) {
           nxt += snprintf(nxt, ldb.offset + 1, "%s", ldb.data);
       }
   }

[namsic]

아래 형태의 구현을 검토해 주기 바랍니다.

int main(int argc, char **argv) {
    int buflen = 200;
    char *buf = (char*)malloc(buflen);
    if (!buf) {
        return 1;
    }

    int offset = 0;
    int i;
    for (i=0; i<20; i++) {
        offset += snprintf(buf + offset, buflen - offset, "%d,", i);
        //if (offset >= buflen) {
        //    break;
        //}
    }

    printf("%s\n", buf);
    free(buf);
    return 0;
}

[cheesecrust]

아래 형태의 구현을 검토해 주기 바랍니다.

int main(int argc, char **argv) {
    int buflen = 200;
    char *buf = (char*)malloc(buflen);
    if (!buf) {
        return 1;
    }

    int offset = 0;
    int i;
    for (i=0; i<20; i++) {
        offset += snprintf(buf + offset, buflen - offset, "%d,", i);
        //if (offset >= buflen) {
        //    break;
        //}
    }

    printf("%s\n", buf);
    free(buf);
    return 0;
}

위를 검토하여 아래와 같이 변경하였습니다.

   if (result.value != NULL) {
       int offset = 0;
       for (int i = 0; i < LQ_CMD_NUM; i++) {
           struct lq_detect_buffer ldb = lqdetect.buffer[i];
           offset += snprintf(result.value + offset, bytes - offset, "%s : %u\n", command_str[i], ldb.ntotal);
           if (ldb.ntotal != 0) {
               offset += snprintf(result.value + offset, ldb.offset + 1, "%s", ldb.data);
           }
       }
       result.length = offset;
   }

[jhpark816]

리뷰 완료

[jhpark816]

위의 코드는 struct lq_detect_buffer 자체를 copy하는 것이므로,
아래와 같이 pointer 사용합시다.

struct lq_detect_buffer *ldb = &lqdetect.buffer[i];

[cheesecrust]

반영하였습니다.

[jhpark816]

코멘트입니다.

• hdrlen 변수를 따로 둘 필요가 없을 것 같고,
• bytes 대신에 length 용어가 나은 것 같고,
• size 출력 인자는 항상 값이 설정되면 좋겠음.

거의 동일한 코드이지만, 아래 코드이면 좋겠습니다.

char *lqdetect_result_get(int *size)
{
    int offset = 0;
    int length = 32 * LQ_CMD_NUM; // header length
    char *str;

    pthread_mutex_lock(&lqdetect.lock);
    for (int i = 0; i < LQ_CMD_NUM; i++) {
        length += lqdetect.buffer[i].offset;
    }
    str = (char*)malloc(length);
    if (str != NULL) {
        for (int i = 0; i < LQ_CMD_NUM; i++) {
            struct lq_detect_buffer *ldb = &lqdetect.buffer[i];
            offset += snprintf(str + offset, length - offset, "%s : %u\n", command_str[i], ldb->ntotal);
            if (ldb->ntotal > 0) {
                offset += snprintf(str + offset, length - offset, "%s", ldb->data);
            }
        }
    }
    pthread_mutex_unlock(&lqdetect.lock);

    *size = offset;
    return str;
}

[jhpark816]

@namsic
참고 사항으로,
아래 buffer 구조에서 keypos, keylen 필드가 없다면,
하나의 buffer 구조 크기는 24 바이트이면 충분하다.

struct lq_detect_buffer {
    char *data;
    uint32_t offset;
    uint32_t ntotal;
    uint32_t nsaved;
    uint32_t keypos[LQ_SAVE_CNT];
    uint32_t keylen[LQ_SAVE_CNT];
};

buffer 크기가 작다면(or 필요한 필드만 복사한다면),
lqdect locking 상태에서

• LQ_CMD_NUM(10) 개의 buffer를 그대로 copy해 두고,
• refcount 증가시켜 둔 다음

without locking 상태에서

• 복사해 둔 buffer 정보를 기반으로 str 변수를 빌드한 후

다시 lqdect locking 상태에서 recount 감소시킨 후에
str 변수를 리턴할 수 있다.

이렇게 최적화할 필요는 없겠죠?

[namsic]

lock을 잡고있는 시간을 최소화하기 위한 관점에서의 최적화인가요?

제 생각에 str 변수를 빌드하는 것이 특별히 시간을 많이 필요로 하는 동작은 아닌 것 같아서,
구현을 단순하게 가져가는 편이 나을 것으로 생각합니다.