feat!: setup build env for kvssd and add skeleton code for kvssd-hashmap interface (re-request) #5
Conversation
| @@ -0,0 +1,90 @@ | |||
| #include <string> | |||
| #include <mutex> | |||
| #include "kvssd_hashmap_db.h" | |||
There was a problem hiding this comment.
include도 convention을 맞춰주셨으면 합니다!
https://stackoverflow.com/questions/691079/is-there-a-standard-include-convention-for-c
There was a problem hiding this comment.
[질문] 약간 다른 주제이지만 mutex의 경우 KVSSD_Hashmap 클래스의 생성자와 소멸자에 적용시킬 예정이었는데, 두 곳 모두 별다른 코드를 넣을 필요성을 찾지 못해서 이 경우 mutex lock은 사용하지 않아도 될까요?
There was a problem hiding this comment.
클래스 생성자와 소멸자에 Mutex가 필요할 필요가 있을까요? Mutex는 Critical Section에만 넣어주면 될 것 같습니다!! unordered_map이 thread safe인지 아닌지를 확인하면 좋을 것 같습니다.
https://stackoverflow.com/questions/9685486/unordered-map-thread-safety
There was a problem hiding this comment.
unordered_map 자체가 thread-safe가 아니었군요...... 읽고 쓰는 부분에 mutex 추가하곘습니다.
There was a problem hiding this comment.
multithread 관련하여 gtest를 추가하였습니다. (아직 repo에 PR하지는 않음)
본 테스트 과정에서 문제를 발견하였습니다. KVSSD 내부적으로는 API 동작 시 병렬성에 문제가 없는데, hashmap kvssd를 위해 사용되는 db_impl.cc에서 API를 실행하기 전 key-value struct를 unique pointer 형태로 만들 때 문제가 발생합니다. 이 unique pointer를 함수 밖에서도 유지할 수 있도록 vector list에 해당 포인터를 push_back 하는 방식을 사용하는데, 해당 부분에서 병렬성이 지켜지지 않았습니다.
해당 부분(vector push_back)을 mutex lock으로 감싸주면 병렬성 문제는 해결되지만 multithread를 사용하지 않는 모든 test에서도 시간이 10배정도 더 걸리는 것을 확인할 수 있었는데, 이 경우 그대로 두어도 괜찮을까요?
(대략 100,000개 정도의 key value API 동작에 2초 가량 소요됩니다.)
There was a problem hiding this comment.
이 unique pointer를 함수 밖에서도 유지할 수 있도록 vector list에 해당 포인터를 push_back 하는 방식을 사용하는데, 해당 부분에서 병렬성이 지켜지지 않았습니다.
GetInstance().memoryList.push_back(std::move(memory)); 부분을 말씀하시는 것일까요?
(대략 100,000개 정도의 key value API 동작에 2초 가량 소요됩니다.)
혹시 가능하시다면 RocksDB나 다른 DB에서 몇 초 정도 걸리는지 확인이 가능할까요?
There was a problem hiding this comment.
GetInstance().memoryList.push_back(std::move(memory));부분을 말씀하시는 것일까요?
네, 맞습니다.
혹시 가능하시다면 RocksDB나 다른 DB에서 몇 초 정도 걸리는지 확인이 가능할까요?
확실하게 이해를 잘 못했는데, 제가 만든 KV API를 현재 hashmap을 다른 DB로 대체해서 테스트하는 걸 말씀하시는 건가요?
There was a problem hiding this comment.
[BlaCkinkGJ] [오후 11:11] 혹시 가능하다면 상위 레이어에서 RocksDB를 돌려보시면 좋을 것 같아요. 안 그러면 아마도 개발에 공수가 좀 더 들어갈 것 같아요.
[BlaCkinkGJ] [오후 11:11] 현재 YCSB Interface -> DB -> DB_internal로 되어있잖아요.
[BlaCkinkGJ] [오후 11:12] 여기서 YCSB Interface -> RocksDB로 I/O 성능을 확인하고
YCSB Interface -> KVSSD DB -> ...로 성능을 확인하는 것이죠
[easter423] [오후 11:19] 그러면 gtest 말고 YCSB에서 원래 제공하는 workload 사용해서 실행하는 거로 이해하면 될까요?
[BlaCkinkGJ] [오후 11:20] 그래도 좋을 것 같고요. 그게 귀찮다면 gtest로 하시는 것도 괜찮아요!!
[BlaCkinkGJ] [오후 11:20] 편하신 방법으로 해보시죠.
[easter423] [오후 11:20] 아 두 방법 전부 상관없군요
[BlaCkinkGJ] [오후 11:21] 다름 아니라, 일반적인 DB 대비해서 성능이 어느정도인지를 확인하고 싶은 상황입니다.
100,000개에 2초면 50,000 IOPS라고 볼 수 있을 것 같은데, 이렇게 나오는 것이 옳은 것이 확인하기 위함이에요!
추가로 만약에 성능이 필요하다면 lock-free list도 찾아보면 좋을 것 같습니다.
There was a problem hiding this comment.
그리고 병렬성이 아니라 동시성을 말씀하시는 것 같습니다.
kvssd/test/kvssd_test.cc
Outdated
| std::string RandomPrintStr(size_t length) | ||
| { | ||
|
|
||
| std::string randomString; | ||
| randomString.reserve(length); | ||
| for (size_t i = 0; i < length; ++i) | ||
| { | ||
| randomString += utils::RandomPrintChar(); | ||
| } | ||
| return randomString; | ||
| } | ||
|
|
||
| std::vector<std::string> key = [] | ||
| { | ||
| std::vector<std::string> keys(NUM_KEYS); | ||
| for (size_t i = 0; i < NUM_KEYS; i++) | ||
| { | ||
| keys[i] = "key" + std::to_string(i); | ||
| } | ||
| return keys; | ||
| }(); | ||
|
|
||
| std::vector<std::vector<DB::Field>> value = [] | ||
| { | ||
| srand(static_cast<unsigned>(time(0))); | ||
| std::vector<std::vector<DB::Field>> values(NUM_VALUES); | ||
| for (size_t i = 0; i < NUM_VALUES; i++) | ||
| { | ||
| values[i] = { | ||
| {"field" + std::to_string(i) + "_1", "value" + std::to_string(i) + RandomPrintStr(32)}, | ||
| {"field" + std::to_string(i) + "_2", "value" + std::to_string(i) + RandomPrintStr(32)}, | ||
| {"field" + std::to_string(i) + "_3", "value" + std::to_string(i) + RandomPrintStr(32)}}; | ||
| } | ||
| return values; | ||
| }(); | ||
|
|
||
| std::vector<DB::Field> output_value; | ||
|
|
||
| std::unique_ptr<KVSSD> kvssd(NewKvssdDB()); |
There was a problem hiding this comment.
외부 파일에서 참고하지 않는 내용들은 static 사용 혹은 anonymous namespace로 감싸주시길 바랍니다.
There was a problem hiding this comment.
anonymous namespace로 감싸는 방식으로 진행하겠습니다.
There was a problem hiding this comment.
https://github.com/google/googletest/blob/main/googletest/samples/sample10_unittest.cc
namespace 용례 참고입니다. 제가 부분만을 일단은 클릭했지만 전체적으로 감싸주는 것이 좋아요.
There was a problem hiding this comment.
말씀해주신 내용처럼 기존 ycsbc namespace는 지우고 anonymous namespace를 전체적으로 TEST 영역 포함하여 감싼 뒤 ycsbc에 대해서는 의존적인 내용에 대해 각각 명시적으로 표현하려고 하였는데, ycsbc도 DB::Field, utils 등 사용되는 곳이 많다보니 자칫 코드가 길어질 수 있겠다는 생각이 듭니다. 이대로 진행해도 상관 없을까요?
There was a problem hiding this comment.
즉 길어진다는 것이 한 줄이 길어진다는거죠? ycsbc::DB::Field가 될까봐 우려하시는 거라면 상관없습니다. 오히려 이렇게 자료형이 명확한 편이 좀 더 좋습니다. DB::Field라고 하면 ycsbc의 Field인지 저희가 따로 만든 커스텀 클래스인지 구분이 안 가는 것이 좀 더 걱정되는 사항이죠.
There was a problem hiding this comment.
지금까지 kvssd_hashmap_db_impl.cc와 헤더 파일에서 제가 만든 class, 함수들은 모두 ycsbc에 포함하였는데(전체를 namespace ycsbc에 감싸는 방식으로) 해당 개체들은 그대로 ycsbc namespace에 유지하는 것보다는 새 namespace를 만드는 것이 나을까요??
아마 ycsbc로 묶어주는 것이 좋을 것 같습니다. lmdb와 leveldb 둘 다, namespace ycsbc { .. } 로 묶어 주네요. 즉, 외부로 노출되는 친구들은 namespace ycsbc로 묶어주시고, 노출되지 않는 것은 anonymous namespace로 묶어주시면 됩니다! 새로운 namespace가 좀 더 명시적으로 보이기 때문에 그렇게 하는 것이 좋을 것 같습니다. 예를 들어 kvssd_hashmap이 좋을 것 같네요.
There was a problem hiding this comment.
kvssd_hashmap namespace로 묶는 것을 kvssd_hashmap_db.h, kvssd_hashmap_db.cc, kvssd_hashmap_db_impl.h, kvssd_hashmap_db_impl.cc 4개의 파일에 반영하였습니다.
기존 ycsbc namespace로 묶은 것은 풀어준 뒤 필요한 부분에 대해서는 ycsbc::로 명시적으로 표기하였습니다.
다만 KVSSD interface같이 hashmap에 국한되지 않는 부분에 대해서는 kvssd_hashmap namespace로 묶는 것이 어울리지 않다고 보는데, 이때는 어떻게 처리하는 것이 좋나요? (일단 kvssd_hashmap namespace에 추가하였습니다.)
There was a problem hiding this comment.
이럴 때에는 저는 보통 best practice로 삼을만한 코드를 확인합니다. LevelDB의 경우에는 namepsace로 묶기는 하군요.
인터페이스에 관해서는 kvssd_hashmap이라는 이름은 좀 이상하니까 namespace kvssd가 어떨까요? 그리고 파일을 kvssd.cc로 분리하는 것도 좋을 것 같습니다.
There was a problem hiding this comment.
감사합니다.
아직 인터페이스 외의 구현은 없어서 kvssd.cc 대신 헤더파일만 만들어도 상관 없을까요?
|
혹시 formatter 뭐 사용하시나요? (e.g., clang-format) 추가로 아직 resolved 처리가 되지 않은 내용들은 처리가 완료되어야 한다는 의미예요 🙂 |
현재 vscode 확장 프로그램으로 ms-vscode.cpptools 를 사용하고 이는 내부적으로 clang-format 포맷터를 사용한다고 하는데, 세부적으로 어떤 스타일로 지정할지는 Clang-Format 확장 프로그램을 따로 깔아야 하는 것으로 보여요
|
제 저장소의 main 브랜치에서는 pull 받을 내용이 없다고 나오는데 혹시 어떤 main 브랜치에서 pull을 받아야 하는지 알려주실 수 있으신가요?? 😅 |
현재 개발용 브랜치 이름이 main이 아니라 master(구 git의 개발용 브랜치)이군요.. |
vscode의 formatter가 동작 중이라면 상관없는 것 같은데... 몇몇 부분은 formatter가 동작하지 않은 듯하게 보이는 부분이 있어보여서요. 저도 일반적으로는 clang-format을 사용합니다. 이번 브랜치 병합하고 적당한 포맷 규칙을 정해봐야겠군요. |
…ashmap_db_run.cc files to implement a new KVSSD interface using a hashmap feat(kvssd): add a kvssd.properties file to configure the kvssd database
…on and deallocation feat(kvssd): add wrapper functions for Read, Insert, Update, Delete operations to simplify usage feat(kvssd): add debug function to print error messages and function execution status feat(kvssd): add functions to print kvs_value and Field vector values for debugging purposes
…g and testing of the kvssd project feat(kvssd/kvssd_hashmap_db.cc): implement missing API functions for the Hashmap_KVSSD class feat(kvssd/kvssd_hashmap_db.h): add missing API functions and struct definitions for the Hashmap_KVSSD class feat(kvssd/kvssd_hashmap_db_impl.h): add implementation details for the Hashmap_KVSSD class feat(kvssd/kvssd_hashmap_db_run.cc): implement missing API functions for the Hashmap_KVSSD class feat(kvssd/test/kvssd_test.cc): add test cases for the Hashmap_KVSSD class using Google Test framework
…ashmap_db_impl.cc fix(kvssd): reformat kvssd_hashmap_db_impl.cc
…nctions style(test/kvssd_test.cc): rename existing tests style(makefile): remove unused comments
…cessary indirection and improve performance feat(kvssd_test.cc): change instantiation of KVSSD to use Hashmap_KVSSD directly to improve performance and reduce indirection
feat(kvssd): add support for ReadersWriterLock in kvssd_hashmap_db.cc to improve concurrency feat(kvssd): add support for ReadersWriterLock in kvssd_hashmap_db.h to improve concurrency
아래 작업을 통해 제 원격 repo 업데이트 완료하였습니다. |
…ey and value parameters before processing requests feat(kvssd_hashmap_db.h): add ValidateRequest function declaration to the KVSSD interface feat(kvssd_const.h): add new header file to define constants for key and value length limits and alignment unit
…improve test coverage feat(test/kvssd_test.cc): add support for ycsbc namespace to improve code organization feat(test/kvssd_test.cc): update test cases to use ycsbc namespace to improve code consistency feat(test/kvssd_test.cc): add support for ycsbc::utils::Exception to improve error handling feat(test/kvssd_test.cc): update test cases to use ycsbc::utils::Exception to improve error handling feat(test/kvssd_test.cc): add support for ycsbc::InsertRow and ycsbc::UpdateRow to improve code organization feat(test/kvssd_test.cc): update test cases to use ycsbc::InsertRow and ycsbc::Update
… kvs_result to improve semantics feat(kvssd/kvssd_hashmap_db.h): add extern const char *kvstrerror[]; to provide error messages for kvs_result values feat(kvssd/kvssd_hashmap_db_impl.cc): update CheckAPI function to use kvs_result::KVS_SUCCESS and static_cast for kvstrerror array indexing to improve compatibility with enum class kvs_result
|
조금만 더 고치면 바로 LGTM 날릴 수 있도록 하겠습니다!! |
… Hashmap_KVSSD class fix(kvssd_hashmap_db.h): delete ValidateRequest virtual method to private from KVSSD interface
… the code feat(kvssd_hashmap_db.h): add namespace kvssd_hashmap to encapsulate the code feat(kvssd_hashmap_db_impl.cc): add namespace kvssd_hashmap to encapsulate the code feat(kvssd_hashmap_db_impl.h): add namespace kvssd_hashmap to encapsulate the code feat(kvssd_test.cc): add namespace kvssd_hashmap to encapsulate the code feat(kvssd_test.cc): update all function calls to use the new namespace
… is no longer needed feat(kvssd/kvssd_hashmap_db.cc): replace ReadersWriterLock with pthread_rwlock_t for improved performance and compatibility feat(kvssd/kvssd_hashmap_db.h): add pthread.h and condition_variable include for pthread_rwlock_t support
There was a problem hiding this comment.
kvssd_hashmap_db.h -> (kvssd_hashmap_db.h, kvssd.h)로 분리해야 합니다.
interface와 기본이 되는 자료구조에 해당하는 친구는 kvssd.h로 이동하고, interface가 아닌 친구들은 kvssd_hashmap_db.h로 가면 됩니다.
There was a problem hiding this comment.
그리고 kvssd.cc 파일을 만들어주는데요. 이 친구는 간단하게 다음처럼 만들면 될 것 같아요.
namespace {
const std::string PROP_BACKEND= "kvssd.backend";
const std::string PROP_BACKEND_DEFAULT = "kvssd";
} // anonymous
DB *NewKvssdDB() {
// 어쩌고 저쩌고
// https://github.com/veritross/YCSB-cpp/blob/86ee150e8946bcac2a6b909d8fef4939d8d5ae61/lmdb/lmdb_db.cc#L58-L88
string backend = ~~;
DB *ret = nullptr;
if (backend == "hashmap") {
// constructor로 초기화
ret = new HashMap("test");
}
if (backend == "kvssd") {
ret = new Kvssd("test", "test");
}
return ret;
}
const bool registered = DBFactory::RegisterDB("kvssd", NewKvssdDB);
Summary
YCSB 내 KVSSD 빌드 환경 구성 및 KVSSD 호환 인터페이스 스켈레톤 코드 작성
Changelog
YCSB-cpp/kvssd)std::unordered_map을 활용한 KVSSD 호환 인터페이스 스켈레톤 코드 작성YCSB-cpp/kvssd/kvssd_hashmap_db.h,YCSB-cpp/kvssd/kvssd_hashmap_db.ccYCSB-cpp/kvssd/kvssd_hashmap_db_run.ccg++ -o kvssd ./kvssd_hashmap_db_run.cc ./kvssd_hashmap_db.cc->./kvssdTODO
Read, Insert, Update, Delete)void* value에서 값을 확인할 수 있도록)Attachments
Refs