[
https://issues.apache.org/jira/browse/GEODE-8102?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17172724#comment-17172724
]
ASF GitHub Bot commented on GEODE-8102:
---------------------------------------
pdxcodemonkey commented on a change in pull request #630:
URL: https://github.com/apache/geode-native/pull/630#discussion_r466728135
##########
File path: cppcache/src/TcpConn.cpp
##########
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+ return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+ return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 16000000) {
+ return 16000000;
+ } else if (pageSize > 0) {
+ return pageSize + (16000000 / pageSize) * pageSize;
+ }
+
+ return 16000000;
Review comment:
Can I get a named constant for this, please? Do you know what
significance the number 16 million has here? Also, isn't (16000000 / pageSize)
* pageSize just always 16000000, so we're just adding 16000000 to pageSize on
line 263?
##########
File path: cppcache/src/TcpConn.cpp
##########
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+ return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+ return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 16000000) {
+ return 16000000;
+ } else if (pageSize > 0) {
+ return pageSize + (16000000 / pageSize) * pageSize;
+ }
+
+ return 16000000;
Review comment:
So line 263 is some misguided attempt to use integer division to our
advantage, but I'm pretty sure it's wrong, unless for some reason we know the
Geode server is doing the exact same bizarre calculation and we need to
replicate it. What the heck is this used for?
##########
File path: cppcache/src/TcpConn.hpp
##########
@@ -65,75 +62,33 @@ class APACHE_GEODE_EXPORT TcpConn : public Connector {
virtual void createSocket(ACE_HANDLE sock);
+ virtual ssize_t doOperation(const SockOp& op, void* buff, size_t sendlen,
+ ACE_Time_Value& waitTime, size_t& readLen) const;
+
public:
- size_t m_chunkSize;
-
- static size_t getDefaultChunkSize() {
- // Attempt to set chunk size to nearest OS page size
- // for perf improvement
- auto pageSize = boost::interprocess::mapped_region::get_page_size();
- if (pageSize > 16000000) {
- return 16000000;
- } else if (pageSize > 0) {
- return pageSize + (16000000 / pageSize) * pageSize;
- }
-
- return 16000000;
- }
-
- TcpConn(const char* hostname, int32_t port,
+ TcpConn(const std::string& hostname, uint16_t port,
std::chrono::microseconds waitSeconds, int32_t maxBuffSizePool);
- TcpConn(const char* ipaddr, std::chrono::microseconds waitSeconds,
+
+ TcpConn(const std::string& address, std::chrono::microseconds waitSeconds,
int32_t maxBuffSizePool);
- virtual ~TcpConn() override { close(); }
+ ~TcpConn() override {}
Review comment:
Why lose the call to `close()` here? Is it called in the base dtor?
##########
File path: cppcache/src/TcpConn.cpp
##########
@@ -318,14 +237,35 @@ size_t TcpConn::socketOp(TcpConn::SockOp op, char *buff,
size_t len,
return totalsend;
}
}
+ssize_t TcpConn::doOperation(const TcpConn::SockOp& op, void* buff,
+ size_t sendlen, ACE_Time_Value& waitTime,
+ size_t& readLen) const {
+ if (op == SOCK_READ) {
+ return stream_->recv_n(buff, sendlen, &waitTime, &readLen);
+ } else {
+ return stream_->send_n(buff, sendlen, &waitTime, &readLen);
+ }
+}
// Return the local port for this TCP connection.
uint16_t TcpConn::getPort() {
ACE_INET_Addr localAddr;
- m_io->get_local_addr(localAddr);
+ stream_->get_local_addr(localAddr);
return localAddr.get_port_number();
}
+size_t TcpConn::getDefaultChunkSize() {
+ //
+ auto pageSize = boost::interprocess::mapped_region::get_page_size();
+ if (pageSize > 16000000) {
+ return 16000000;
+ } else if (pageSize > 0) {
+ return pageSize + (16000000 / pageSize) * pageSize;
+ }
+
+ return 16000000;
Review comment:
Okay, the comment deleted from the original gives it away:
// Attempt to set chunk size to nearest OS page size
// for perf improvement
Spoiler alert, this will NOT improve perf in any perceptible way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Link and load OpenSSL library directly
> --------------------------------------
>
> Key: GEODE-8102
> URL: https://issues.apache.org/jira/browse/GEODE-8102
> Project: Geode
> Issue Type: Improvement
> Components: native client
> Reporter: Jacob Barrett
> Priority: Major
> Labels: pull-request-available
>
> Lazy load the OpenSSL library directly, through ACE_SSL, into the
> apache-geode library. Currently we lazy load cryptoImpl, which immediately
> loads OpenSSL. The original intent was to avoid having an immediate
> dependency on OpenSSL at a time when its availability was questionable. On
> unix like systems OpenSSL is almost always available since so many other
> components in the OS depend on it. This immediate load dependency will have
> little to no effect on those systems. On some unix like systems the
> experience will improve by not having a runtime dependency on an intermediate
> library, cryptoImpl, that may need special treatments, like LD_LIBRARY_PATH
> or RPATH changes. On Windows, where OpenSSL is an anomaly we can use MSVC's
> lazy loading feature to only load OpenSSL if SSL/TLS is configured. This
> significantly improves the experience on Windows with regards to the location
> of cryptoImpl when using .NET.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
