Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add command object pool #260

Closed
wants to merge 4 commits into from

Conversation

lqxhub
Copy link
Collaborator

@lqxhub lqxhub commented Apr 6, 2024

使用对象池来解决命令对象复用, 现在只是一个实现方案的思路验证,还请帮忙多检查

对象复用池在 cmd_object_pool 文件, 这个会代替 cmd_table_manager , 后续相关的文件会删除.

@github-actions github-actions bot added the ✏️ Feature New feature or request label Apr 6, 2024
}

thread_local std::unique_ptr<std::vector<SmallObject>> CmdObjectPool::local_pool = nullptr;
thread_local uint64_t CmdObjectPool::counter = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

放到文件开头定义? 然后就是这变量要不要加一个g_this_之类的前缀,看的有点迷糊

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

加个 tl_ 前缀怎么样

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以

// if you don t find it go to the global search
return GetObjectByGlobal(key);
}
void CmdObjectPool::PutObject(std::string &&key, std::unique_ptr<BaseCmd> &&v) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

加个空行

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

++counter;
if (counter == 0) {
// If the version number reaches 0, reset all version numbers in localPool
for (auto &obj : *local_pool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前的代码逻辑似乎counter不会为0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

counteruint64 类型, 当 counter 累加到最大值后,再+1, 这时候会溢出, 然后成0, 当然这种情况很难出现,或者说几乎不会出现


// If you can't find it, use the func function to create a corresponding object
auto func = create_object_.find(key);
if (func == create_object_.end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是否可以减小锁mutex_的粒度,目前来看create_object_不需要锁保护

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

对的, 可以减小

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// std::cout << "smallPool.size:" << local_pool->size() << " smallPool.cap:" << local_pool->capacity() <<
// std::endl;
if (local_pool->size() <= local_max_) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是否考虑如果使用次数少于多少也把它移动到全局的命令对象池中?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样做的目的是什么呢

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考虑的是使用次数少到一定程度的命令对象放到全局对象池中,可以供其他的线程使用,减少创建的次数,另一方面就是可以减少这个线程对应的命令对象占用内存大小

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个加了, 现在会在每3次(随便定的)回收的时候, 判断一下, 如果有使用次数是1的命令, 也会进行回收

bool needPush = true;
for (auto &obj : *local_pool) {
if (obj.key_ == key && !obj.object_) {
// if the keys are the same and the pointer is empty
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这种情况会在什么前提下发生,按照道理来说不应该是成对出现,为啥会出现命令对象为null的情况

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

现在的使用场景这部分可以去掉了, 之前想的是在 IO线程中获取命令对象, 然后把命令传到 线程池中使用, 这时候可能会出现多个同名命令的情况,比如 第一个命令是 GET, 获取了一个 GetCmd 对象, 然后这个命令还没有执行完成, 又来了一个 GET 命令, 这时候就需要再从全局里获取一个 GetCmd对象, 此时 local_pool 中就有两个 GetCmd 对象了,

现在把命令对象获取和释放, 都放在线程池中了, 可以避免这个问题, 就是还没想好, 这个地方该怎么处理, 要不要放在 IO线程里

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的

@AlexStocks
Copy link
Contributor

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Robustness
Implement error handling in object pool methods to enhance robustness

Add error handling for the GetObject and PutObject methods in the CmdObjectPool class to
manage exceptions or failures in object retrieval and storage, enhancing robustness.

src/cmd_object_pool.h [44-47]

-std::unique_ptr<BaseCmd> GetObject(const std::string &key);
-void PutObject(std::string &&key, std::unique_ptr<BaseCmd> &&v);
+std::unique_ptr<BaseCmd> GetObject(const std::string &key) {
+  try {
+    // existing implementation
+  } catch (const std::exception& e) {
+    // handle error
+  }
+}
+void PutObject(std::string &&key, std::unique_ptr<BaseCmd> &&v) {
+  try {
+    // existing implementation
+  } catch (const std::exception& e) {
+    // handle error
+  }
+}
 
Suggestion importance[1-10]: 9

Why: Adding error handling to these methods significantly enhances the robustness of the code by managing exceptions and failures, which is crucial for reliable software.

9
Enhancement
Add validation for flag values before applying bitwise operations

Consider checking the result of bitwise operations for flags to ensure they are valid
before applying them. This can prevent unintended behavior due to incorrect flag values.

src/base_cmd.cc [54-55]

-void BaseCmd::SetFlag(uint32_t flag) { flag_ |= flag; }
-void BaseCmd::ResetFlag(uint32_t flag) { flag_ &= ~flag; }
+void BaseCmd::SetFlag(uint32_t flag) {
+  if (isValidFlag(flag)) {
+    flag_ |= flag;
+  }
+}
+void BaseCmd::ResetFlag(uint32_t flag) {
+  if (isValidFlag(flag)) {
+    flag_ &= ~flag;
+  }
+}
 
Suggestion importance[1-10]: 8

Why: Adding validation for flag values before applying bitwise operations can prevent unintended behavior due to incorrect flag values. This is a significant improvement for ensuring the robustness of the code.

8
Improve error handling by adding logging when a command is not found

It is recommended to handle the case where cmdPtr is nullptr more robustly by logging the
error or handling the specific case inside the if (!cmdPtr) block.

src/cmd_thread_pool_worker.cc [21-30]

 if (!cmdPtr) {
   if (ret == CmdRes::kInvalidParameter) {
     task->Client()->SetRes(CmdRes::kInvalidParameter);
   } else {
+    LOG_ERROR("Command not found: " + task->CmdName());
     task->Client()->SetRes(CmdRes::kErrOther, "unknown command '" + task->CmdName() + "'");
   }
   g_pikiwidb->PushWriteTask(task->Client());
   continue;
 }
 
Suggestion importance[1-10]: 7

Why: Adding logging when a command is not found improves error handling and makes debugging easier. However, it is a minor enhancement and not critical to the functionality.

7
Replace placeholder implementation with actual logic for configuration commands

Implement actual command logic in ConfigGetCmd::DoCmd and ConfigSetCmd::DoCmd instead of
placeholder text to ensure the commands perform their intended functions.

src/cmd_admin.cc [23-30]

-void ConfigGetCmd::DoCmd(PClient* client) { client->AppendString("config cmd in development"); }
-void ConfigSetCmd::DoCmd(PClient* client) { client->AppendString("config cmd in development"); }
+void ConfigGetCmd::DoCmd(PClient* client) {
+  // Example implementation
+  std::string configValue = getConfigValue(client->argv_[1]);
+  client->AppendString(configValue);
+}
+void ConfigSetCmd::DoCmd(PClient* client) {
+  // Example implementation
+  bool success = setConfigValue(client->argv_[1], client->argv_[2]);
+  client->AppendString(success ? "Success" : "Failure");
+}
 
Suggestion importance[1-10]: 6

Why: Implementing actual command logic instead of placeholder text is important for the functionality of the commands. However, the suggestion does not provide the actual logic, only a placeholder for it.

6
Best practice
Initialize the command object pool in the PikiwiDB constructor to ensure it is ready for use

Consider initializing the cmd_object_pool_ in the constructor of PikiwiDB to ensure it is
properly set up before use. This change ensures that the object pool is always ready when
the database instance is created, avoiding potential null pointer dereferences.

src/pikiwidb.cc [318]

 g_pikiwidb = std::make_unique<PikiwiDB>();
+g_pikiwidb->InitCmdObjectPool();
 
Suggestion importance[1-10]: 8

Why: Initializing cmd_object_pool_ in the constructor is a good practice to ensure it is set up before use, preventing potential null pointer dereferences. This suggestion improves the robustness of the code.

8
Maintainability
Remove commented-out code for clarity and maintainability

Remove the commented-out initialization of g_cmd_object_pool in the main function, as it
seems to be handled elsewhere in the code. This cleanup will improve code readability and
maintainability.

src/pikiwidb.cc [319]

-//  g_cmd_object_pool = std::make_unique<pikiwidb::CmdObjectPool>();
+// Initialization of g_cmd_object_pool is handled in the PikiwiDB class.
 
Suggestion importance[1-10]: 7

Why: Removing commented-out code improves readability and maintainability. This suggestion is correct and helps keep the codebase clean.

7
Performance
Optimize the global pool management for better performance in concurrent environments

Consider using a lock-free data structure or fine-grained locking mechanism for managing
the global pool in CmdObjectPool to improve performance under high concurrency.

src/cmd_object_pool.h [70-74]

-std::unordered_map<std::string, std::unique_ptr<BaseCmd>> pool_;
-std::mutex mutex_;
+// Use concurrent data structure or fine-grained locks
+ConcurrentMap<std::string, std::unique_ptr<BaseCmd>> pool_;
 
Suggestion importance[1-10]: 6

Why: While this suggestion could improve performance under high concurrency, it requires careful consideration and testing. It is a good suggestion but not immediately crucial.

6
Optimize the cleanup logic in the object pool by using a min-heap for better performance

Optimize the object pool cleanup logic by introducing a more efficient algorithm for
managing rarely used objects, potentially using a min-heap for tracking object usage.

src/cmd_object_pool.cc [213-223]

-for (auto it = tl_local_pool->rbegin(); it != tl_local_pool->rend();) {
-  if (reclaim_rarely_used && it->count_ > 1) {
-    break;
+// Using min-heap to manage object usage efficiently
+if (reclaim_rarely_used) {
+  std::make_heap(tl_local_pool->begin(), tl_local_pool->end(), [](const SmallObject &a, const SmallObject &b) {
+    return a.count_ < b.count_;
+  });
+  while (tl_local_pool->size() > local_max_ && tl_local_pool->front().count_ <= 1) {
+    PutObjectBackGlobal(tl_local_pool->front().key_, tl_local_pool->front().object_);
+    std::pop_heap(tl_local_pool->begin(), tl_local_pool->end(), [](const SmallObject &a, the SmallObject &b) {
+      return a.count_ < b.count_;
+    });
+    tl_local_pool->pop_back();
   }
-  PutObjectBackGlobal(it->key_, it->object_);
-  it = std::vector<SmallObject>::reverse_iterator(tl_local_pool->erase((++it).base()));
 }
 
Suggestion importance[1-10]: 5

Why: Using a min-heap to manage object usage can improve performance, but the current implementation may already be sufficient for the intended use case. This is a performance optimization rather than a critical fix.

5

@AlexStocks
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, because the PR introduces a significant amount of new code across multiple files, involving complex logic related to command object pooling and thread-local storage. The changes impact core functionality and require careful review to ensure thread safety, memory management, and correct behavior across different command types.

🧪 Relevant tests

No

⚡ Possible issues

Thread Safety: The use of thread_local for tl_local_pool and tl_counter in CmdObjectPool requires careful handling to ensure that objects are managed correctly across different threads.

Memory Management: The manual management of memory using std::unique_ptr in multiple places, especially in PutObject and GetObjectByGlobal, could lead to potential memory leaks or double frees if not handled correctly.

🔒 Security concerns

No

Code feedback:
relevant filesrc/cmd_object_pool.cc
suggestion      

Consider using a more robust locking mechanism or lock-free data structures for managing the command object pool to enhance performance and avoid potential deadlocks or race conditions. [important]

relevant linestd::lock_guard l(mutex_);

relevant filesrc/cmd_object_pool.cc
suggestion      

Implement logging for error states and unusual behavior, especially in the GetObject and PutObject methods, to aid in debugging and monitoring the system's behavior. [important]

relevant linereturn std::move(obj.object_);

relevant filesrc/cmd_object_pool.cc
suggestion      

Optimize the GetObjectByGlobal method by checking if the object can be reused without removal from the global pool, which might reduce the overhead of object creation and destruction. [medium]

relevant linepool_.erase(it);

relevant filesrc/cmd_object_pool.cc
suggestion      

Refactor the PutObject method to encapsulate the logic for reclaiming rarely used objects into a separate function to improve modularity and readability. [medium]

relevant linebool reclaim_rarely_used = tl_counter % (check_rate_ * less_use_check_rate_) == 0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants