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.

Sign up for GitHub

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

Add C++17 UDPServer ID handling #260

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add C++17 UDPServer ID handling #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f431aca
Adding C++17 UDPServer ID handling
PioLing Dec 6, 2024
680bb97
Improve the UDP server encapsulation and remove redundant constructor…
PioLing Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/Network/UdpServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@
static constexpr auto kUdpDelayCloseMS = 3 * 1000;

static UdpServer::PeerIdType makeSockId(sockaddr *addr, int) {
UdpServer::PeerIdType ret;
UdpServer::PeerIdType ret(PEERTYPELEN, ' ');
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 @@
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: assert(0); return ret;
}
}

Expand Down Expand Up @@ -78,7 +76,7 @@
//主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 @@
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 Expand Up @@ -277,7 +275,7 @@

assert(_socket);
socket->bindUdpSock(_socket->get_local_port(), _socket->get_local_ip());
socket->bindPeerAddr((struct sockaddr *) addr_str.data(), addr_str.size());

Check warning on line 278 in src/Network/UdpServer.cpp

View workflow job for this annotation

GitHub Actions / build 

 'argument': conversion from 'size_t' to 'int', possible loss of data

auto helper = _session_alloc(server, socket);
// 把本服务器的配置传递给 Session [AUTO-TRANSLATED:e3ed95ab]
Expand Down
41 changes: 39 additions & 2 deletions src/Network/UdpServer.h
View file
Open in desktop
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

Summary

This patch modifies the UdpServer class to improve its handling of peer IDs, leveraging C++17 features for better performance and type safety. It's a patch review focusing on the changes introduced.

Detailed Feedback

Code Overview

The patch introduces a PeerIdType class, using std::array for C++17-compatible systems and std::string otherwise. It also adds a custom hash function for PeerIdType to improve the performance of the _session_map. The _session_map itself is updated to use the new PeerIdType and the custom hash function.

Strengths

  • Improved Type Safety: Using std::array for PeerIdType provides better type safety compared to std::string when C++17 is available.
  • Potential Performance Improvement: The custom hash function for PeerIdType should improve the performance of lookups in the _session_map, especially with a large number of sessions.
  • C++17 Compatibility: The code gracefully handles both C++17 and pre-C++17 environments.
  • Clearer Code Structure: The use of std::array and the custom hash function makes the code more readable and maintainable.

Areas for Improvement

1. PeerIdType Comparison

  • Issue: The operator== in PeerIdType performs a piecemeal comparison of the underlying data as uint64_t and uint16_t. This is less efficient and less readable than a direct memory comparison. It also assumes a specific memory layout which might be compiler-dependent.
  • Suggestion: Use std::memcmp for a more efficient and portable comparison. This avoids potential issues related to endianness and compiler-specific optimizations.
  • Example:
bool operator==(const PeerIdType &that) const {
    return std::memcmp(data(), that.data(), size()) == 0;
}

2. Error Handling in PeerIdType Constructor

  • Issue: The constructor of PeerIdType truncates the input length if it exceeds the array size, but it doesn't provide any indication of this truncation.
  • Suggestion: Consider adding a check and throwing an exception or returning an error code if the input length is invalid. This would make the code more robust.

3. Documentation

  • Issue: The comments in the code are mostly in English and Chinese. While the Chinese comments might be helpful for some developers, it's generally better to stick to a single language for better maintainability and internationalization.
  • Suggestion: Remove the Chinese comments and improve the English comments to be more concise and informative. Focus on explaining the why behind the code, not just the what.

4. PeerIdHash

  • Issue: The PeerIdHash uses std::string_view which might involve unnecessary copying.
  • Suggestion: If performance is critical, consider using a custom hash function that directly operates on the std::array data without creating a temporary std::string_view. This would avoid the overhead of constructing a std::string_view.

Conclusion

This patch is a significant improvement to the UdpServer class, enhancing type safety and potentially improving performance. Addressing the suggested improvements in PeerIdType comparison, error handling, documentation, and the custom hash function would further enhance the code's robustness, readability, and efficiency. The overall approach is well-structured and demonstrates good understanding of C++17 features.

TRANS_BY_GITHUB_AI_ASSISTANT

Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,52 @@
#ifndef TOOLKIT_NETWORK_UDPSERVER_H
#define TOOLKIT_NETWORK_UDPSERVER_H

#if __cplusplus >= 201703L
#include <array>
#include <string_view>
#endif
#include "Server.h"
#include "Session.h"

#define PEERTYPELEN (18)
namespace toolkit {

class UdpServer : public Server {
public:
using Ptr = std::shared_ptr<UdpServer>;
#if __cplusplus >= 201703L
class PeerIdType : public std::array<char, PEERTYPELEN> {
public:
PeerIdType() {
fill(' ');
}
PeerIdType(size_t len, char fillChar) {
//if (len > size()) {
// len = size();
//}
//fill(fillChar);
}
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));
}
};

struct PeerIdHash {
size_t operator()(const PeerIdType& v) const noexcept { return std::hash<std::string_view> {}(std::string_view(v.data(), v.size())); }
};
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr, PeerIdHash>;

#else
using PeerIdType = std::string;
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr>;
#endif

using Ptr = std::shared_ptr<UdpServer>;
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 @@ -150,7 +187,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
Loading