Skip to content

Commit

Permalink
优化UDP链接id生成性能
Browse files Browse the repository at this point in the history
  • Loading branch information
@xia-chu
xia-chu committed Dec 5, 2024
1 parent 0421201 commit df695ad
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 7 deletions.
8 changes: 3 additions & 5 deletions src/Network/UdpServer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ static UdpServer::PeerIdType makeSockId(sockaddr *addr, int) {
UdpServer::PeerIdType ret;
switch (addr->sa_family) {
case AF_INET : {
ret.resize(18);
ret[0] = ((struct sockaddr_in *) addr)->sin_port >> 8;
ret[1] = ((struct sockaddr_in *) addr)->sin_port & 0xFF;
//ipv4地址统一转换为ipv6方式处理 [AUTO-TRANSLATED:ad7cf8c3]
Expand All @@ -35,13 +34,12 @@ static UdpServer::PeerIdType makeSockId(sockaddr *addr, int) {
return ret;
}
case AF_INET6 : {
ret.resize(18);
ret[0] = ((struct sockaddr_in6 *) addr)->sin6_port >> 8;
ret[1] = ((struct sockaddr_in6 *) addr)->sin6_port & 0xFF;
memcpy(&ret[2], &(((struct sockaddr_in6 *)addr)->sin6_addr), 16);
return ret;
}
default: assert(0); return "";
default: throw std::invalid_argument("invalid sockaddr address");
}
}

Expand Down Expand Up @@ -78,7 +76,7 @@ void UdpServer::start_l(uint16_t port, const std::string &host) {
//主server才创建session map，其他cloned server共享之 [AUTO-TRANSLATED:113cf4fd]
//Only the main server creates a session map, other cloned servers share it
_session_mutex = std::make_shared<std::recursive_mutex>();
_session_map = std::make_shared<std::unordered_map<PeerIdType, SessionHelper::Ptr> >();
_session_map = std::make_shared<SessionMapType>();

// 新建一个定时器定时管理这些 udp 会话,这些对象只由主server做超时管理，cloned server不管理 [AUTO-TRANSLATED:d20478a2]
//Create a timer to manage these udp sessions periodically, these objects are only managed by the main server, cloned servers do not manage them
Expand Down Expand Up @@ -207,7 +205,7 @@ void UdpServer::onManagerSession() {
std::lock_guard<std::recursive_mutex> lock(*_session_mutex);
//拷贝map，防止遍历时移除对象 [AUTO-TRANSLATED:ebbc7595]
//Copy the map to prevent objects from being removed during traversal
copy_map = std::make_shared<std::unordered_map<PeerIdType, SessionHelper::Ptr> >(*_session_map);
copy_map = std::make_shared<SessionMapType>(*_session_map);
}
auto lam = [copy_map]() {
for (auto &pr : *copy_map) {
Expand Down
10 changes: 8 additions & 2 deletions src/Network/UdpServer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#ifndef TOOLKIT_NETWORK_UDPSERVER_H
#define TOOLKIT_NETWORK_UDPSERVER_H

#include <string_view>
#include "Server.h"
#include "Session.h"

Expand All @@ -19,7 +20,7 @@ namespace toolkit {
class UdpServer : public Server {
public:
using Ptr = std::shared_ptr<UdpServer>;
using PeerIdType = std::string;
using PeerIdType = std::array<char, 18>;
using onCreateSocket = std::function<Socket::Ptr(const EventPoller::Ptr &, const Buffer::Ptr &, struct sockaddr *, int)>;

explicit UdpServer(const EventPoller::Ptr &poller = nullptr);
Expand Down Expand Up @@ -75,6 +76,11 @@ class UdpServer : public Server {
virtual void cloneFrom(const UdpServer &that);

private:
struct PeerIdHash {
size_t operator()(const PeerIdType &v) const noexcept { return std::hash<std::string_view> {}(std::string_view(v.data(), v.size())); }

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': is not a member of 'std'

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': undeclared identifier

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'std::hash': 'string_view' is not a valid template type argument for parameter '_Kty'

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'std::hash': use of class template requires template argument list

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': is not a member of 'std'

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 use of undefined type 'std::array<char,18>'

Check failure on line 80 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': identifier not found
};
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr, PeerIdHash>;

/**
* @brief 开始udp server
* @param port 本机端口，0则随机
Expand Down Expand Up @@ -150,7 +156,7 @@ class UdpServer : public Server {
//cloned server共享主server的session map，防止数据在不同server间漂移 [AUTO-TRANSLATED:9a149e52]
//Cloned server shares the session map with the main server, preventing data drift between different servers
std::shared_ptr<std::recursive_mutex> _session_mutex;
std::shared_ptr<std::unordered_map<PeerIdType, SessionHelper::Ptr> > _session_map;
std::shared_ptr<SessionMapType> _session_map;
//主server持有cloned server的引用 [AUTO-TRANSLATED:04a6403a]
//Main server holds a reference to the cloned server
std::unordered_map<EventPoller *, Ptr> _cloned_server;
Expand Down

3 comments on commit df695ad

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

Optimize UDP connection ID generation performance

TRANS_BY_GITHUB_AI_ASSISTANT

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Network/UdpServer.cpp:

Code Review: Patch for src/Network/UdpServer.cpp in ZLToolKit

Summary

This patch improves the UDP server's performance by optimizing the PeerId generation and using a more efficient map type. It also replaces an assert with a more robust exception handling mechanism. This is a patch review focusing on the changes introduced.

Detailed Feedback

Code Overview

The patch modifies the makeSockId function to avoid unnecessary resizing of the PeerIdType vector, improving efficiency. It replaces the std::unordered_map<PeerIdType, SessionHelper::Ptr> with a type alias SessionMapType for better readability and maintainability. Finally, it replaces a potentially problematic assert(0) with a throw std::invalid_argument, providing better error handling.

Strengths

  • Improved Efficiency: Removing the unnecessary resize call in makeSockId directly improves performance, especially for a high-volume UDP server.
  • Better Error Handling: Replacing assert(0) with an exception provides more graceful handling of invalid sockaddr addresses. The application can now catch and handle this error instead of crashing.
  • Improved Readability: Using SessionMapType improves code readability and maintainability.

Areas for Improvement

1. SessionMapType Definition

  • Issue: The patch doesn't define SessionMapType. The reviewer needs to infer its definition from context.
  • Suggestion: Explicitly define SessionMapType within the code, either as a typedef or using declaration. This makes the code self-contained and easier to understand.
  • Example:
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr>;

2. Exception Handling in makeSockId

  • Issue: While throwing std::invalid_argument is better than assert(0), consider whether a more specific exception type is appropriate. std::runtime_error might be a better fit, as this is a runtime error condition.
  • Suggestion: Use std::runtime_error or a custom exception type specific to network errors. This allows for more precise error handling in the calling functions.
  • Example:
default: throw std::runtime_error("invalid sockaddr address");

3. Potential for Memory Leaks (Minor)

  • Issue: The onRead_l function uses std::move(buf) to transfer ownership of the buffer. However, if helper_creator fails to create a helper, the buffer is lost. While this is unlikely in normal operation, it's a potential memory leak.
  • Suggestion: Add a check after helper_creator() to ensure a helper was created. If not, handle the cacheable_buf appropriately (e.g., log a warning and release the buffer).

Conclusion

The patch is a positive improvement to the UDP server code. The efficiency gains from optimizing makeSockId are significant. The improved error handling and readability enhancements are also valuable. Addressing the minor points regarding SessionMapType definition and potential memory leaks would further enhance the code's robustness and clarity. The changes are well-integrated and improve the overall quality of the code.

TRANS_BY_GITHUB_AI_ASSISTANT

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Network/UdpServer.h:

Code Review: Patch to src/Network/UdpServer.h

Summary

This patch modifies the UdpServer class in the ZLToolKit project to improve the performance of UDP connection ID generation. The key change is switching from using std::string to std::array<char, 18> for PeerIdType and introducing a custom hash function for better performance with std::unordered_map. This is a patch review.

Detailed Feedback

Code Overview

The patch aims to optimize the performance of the UdpServer class by changing the data type of PeerIdType from std::string to std::array<char, 18> and implementing a custom hash function for the std::unordered_map used to store sessions. This should reduce memory overhead and improve hash table lookup speed.

Strengths

  • Improved Performance: The change to std::array and the custom hash function is likely to significantly improve performance, especially with a large number of concurrent connections. std::array avoids dynamic memory allocation, and a custom hash function tailored to the specific data type can be more efficient than the generic std::hash.
  • Clearer Intent: The addition of comments explaining the purpose of sharing the _session_map between the main server and cloned servers improves code readability and maintainability.
  • Inclusion of <string_view>: The inclusion of <string_view> is a good practice, although its direct use in this patch is limited to the custom hash function. It suggests a potential for further optimization or code simplification elsewhere.

Areas for Improvement

1. Justification for std::array<char, 18>

  • Issue: The patch changes PeerIdType to std::array<char, 18>. The rationale behind choosing 18 characters is not explicitly stated. This size needs justification. What is the maximum length of a UDP peer ID in this system? Is 18 characters sufficient to uniquely identify all potential peers? If a longer ID is needed, this will cause issues. If a shorter ID is sufficient, memory could be saved.
  • Suggestion: Add a comment explaining the choice of 18 characters. Consider adding a constant to define this size for better maintainability. If possible, provide a clear explanation of the ID generation process and why this size is appropriate.
  • Example: 
    // Peer ID length.  Chosen based on [explanation of ID generation and maximum length].
constexpr size_t kPeerIdLength = 18;
using PeerIdType = std::array<char, kPeerIdLength>;

2. Potential for Collision in Custom Hash Function

  • Issue: The custom hash function uses std::hash<std::string_view>. While generally efficient, there's a small risk of hash collisions, especially if the peer IDs aren't uniformly distributed. The quality of the hash function directly impacts the performance of the std::unordered_map.
  • Suggestion: Consider using a more robust hash function, potentially a custom implementation that leverages techniques like MurmurHash or CityHash, known for their low collision rates. Benchmarking different hash functions would be beneficial to determine the optimal choice.
  • Example: (Illustrative - requires actual implementation of a chosen hash function) 
    struct PeerIdHash {
    size_t operator()(const PeerIdType &v) const noexcept { 
        return MurmurHash3_x86_32(v.data(), v.size(), 0); // Replace with chosen hash function
    }
};

3. Error Handling and Exception Safety

  • Issue: The code lacks explicit error handling. What happens if memory allocation fails during session creation? The code should be more robust to handle potential exceptions.
  • Suggestion: Add error handling mechanisms (e.g., using try-catch blocks or checking return values of memory allocation functions) to gracefully handle potential failures and prevent crashes. Consider logging errors for debugging purposes.

Conclusion

The patch is a positive step towards improving the performance of the UdpServer class. The switch to std::array and the custom hash function are likely to yield significant performance gains. However, addressing the points raised regarding the justification for the PeerIdType size, potential hash collisions, and error handling will further enhance the code's robustness and maintainability. Thorough testing and benchmarking are crucial to validate the performance improvements and ensure the correctness of the changes.

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.