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 54a63d9
Show file tree
Hide file tree
Showing 2 changed files with 25 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;

Check failure on line 28 in src/Network/UdpServer.cpp

View workflow job for this annotation

GitHub Actions / build 

 binary '[': 'toolkit::UdpServer::PeerIdType' does not define this operator or a conversion to a type acceptable to the predefined operator

Check failure on line 28 in src/Network/UdpServer.cpp

View workflow job for this annotation

GitHub Actions / build 

 binary '[': 'toolkit::UdpServer::PeerIdType' does not define this operator or a conversion to a type acceptable to the predefined operator
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
24 changes: 22 additions & 2 deletions src/Network/UdpServer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,30 @@
#ifndef TOOLKIT_NETWORK_UDPSERVER_H
#define TOOLKIT_NETWORK_UDPSERVER_H

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

namespace toolkit {

class UdpServer : public Server {
public:
class PeerIdType : public std::array<char, 18> {

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

View workflow job for this annotation

GitHub Actions / build 

 'std::array<char,18>': base class undefined

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

View workflow job for this annotation

GitHub Actions / build 

 'std::array<char,18>': base class undefined
public:
bool operator==(const PeerIdType &that) const {
return as<uint64_t>(0) == that.as<uint64_t>(0) &&
as<uint64_t>(8) == that.as<uint64_t>(8) &&
as<uint16_t>(16) == that.as<uint16_t>(16);
}

private:
template <class T>
const T& as(size_t offset) const {
return *(reinterpret_cast<const T *>(data() + offset));
}
};

using Ptr = std::shared_ptr<UdpServer>;
using PeerIdType = std::string;
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 +90,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 94 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 94 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': undeclared identifier

Check failure on line 94 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 94 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 94 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 94 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'data': is not a member of 'toolkit::UdpServer::PeerIdType'

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

View workflow job for this annotation

GitHub Actions / build 

 'size': is not a member of 'toolkit::UdpServer::PeerIdType'

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

View workflow job for this annotation

GitHub Actions / build 

 'string_view': identifier not found

Check failure on line 94 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 94 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'string_view': undeclared identifier

Check failure on line 94 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 94 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 94 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 94 in src/Network/UdpServer.h

View workflow job for this annotation

GitHub Actions / build 

 'data': is not a member of 'toolkit::UdpServer::PeerIdType'

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

View workflow job for this annotation

GitHub Actions / build 

 'size': is not a member of 'toolkit::UdpServer::PeerIdType'

Check failure on line 94 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 +170,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

4 comments on commit 54a63d9

@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 UdpServer.h in ZLToolKit

Summary

This patch modifies the UdpServer class in the ZLToolKit project to improve the performance of UDP peer ID generation. It replaces the previous std::string based PeerId with a custom PeerIdType using std::array<char, 18>, and introduces a custom hash function for improved performance in the std::unordered_map. This is a patch review.

Detailed Feedback

Code Overview

The patch aims to optimize the handling of UDP peer IDs by switching from std::string to a custom std::array<char, 18> type called PeerIdType. A custom hash function is also introduced to improve the performance of the std::unordered_map used to store sessions.

Strengths

  • Improved Performance: Using std::array instead of std::string for Peer IDs likely improves performance, especially for hash table lookups, due to the predictable memory layout and size of std::array.
  • Custom Hash Function: The introduction of a custom hash function tailored to the PeerIdType is a good approach to optimize hash table performance. This avoids the overhead of the default std::string hash function.
  • Clearer Code Structure: The use of std::string_view in the hash function improves readability and efficiency by avoiding unnecessary string copies.
  • Correctness: The PeerIdType comparison operator ensures correct equality checks.

Areas for Improvement

1. PeerIdType Design and Potential Issues

  • Issue: The PeerIdType is tightly coupled to a specific size (18 bytes). If the underlying data structure generating the Peer ID changes, this class will need modification. The hardcoded access to uint64_t and uint16_t within the as function makes it less flexible.
  • Suggestion: Consider using a more flexible approach, perhaps a template class that takes the size as a template parameter, or a more abstract representation of the peer ID. This would make the code more adaptable to future changes. Consider using std::memcpy for safer and potentially faster data access instead of reinterpret_cast.
  • Example:
template <size_t Size>
class PeerIdType : public std::array<char, Size> {
public:
    bool operator==(const PeerIdType<Size>& that) const {
        return std::memcmp(data(), that.data(), Size) == 0;
    }
};

struct PeerIdHash {
    template <size_t Size>
    size_t operator()(const PeerIdType<Size>& v) const noexcept {
        // Implement a suitable hash function for arbitrary sizes.  Consider using a library like boost::hash_combine
        size_t hash_value = 0;
        for (char c : v) {
            hash_value = hash_value * 31 + c; // Simple example, replace with a better hash function
        }
        return hash_value;
    }
};

2. Error Handling and Robustness

  • Issue: The code lacks error handling. For example, what happens if the socket creation fails? The createSocket function should return an error code or throw an exception to indicate failure.
  • Suggestion: Add error handling to all functions that can potentially fail, such as socket creation and session management. Consider using exceptions or return codes to signal errors.

3. Documentation

  • Issue: While the code has comments, they are mostly auto-translated and could be improved.
  • Suggestion: Provide clear and concise documentation in English for all public functions and classes, explaining their purpose, parameters, return values, and any potential exceptions. Remove or update the auto-translated comments.

Conclusion

The patch improves the performance of UDP peer ID handling. However, the PeerIdType design could be made more flexible and robust. Adding error handling and improving the documentation are crucial for maintainability and reliability. The suggested improvements would enhance the code's long-term viability and adaptability.

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.

Optimize UDP connection ID generation performance

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.