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


Reply via email to