[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #845: MINIFICPP-1277 - Create cross-platform API for querying memory usage

[GitBox]

szaszm commented on a change in pull request #845:
URL: https://github.com/apache/nifi-minifi-cpp/pull/845#discussion_r463076019



##
File path: libminifi/src/utils/OsUtils.cpp
##
@@ -154,6 +164,41 @@ std::string OsUtils::userIdToUsername(const std::string 
) {
   return name;
 }
 
+uint64_t OsUtils::getMemoryUsage() {
+#ifdef __linux__
+  static const std::string linePrefix = "VmRSS:";
+  std::ifstream statusFile("/proc/self/status");
+  std::string line;
+
+  while (std::getline(statusFile, line)) {
+// if line begins with "VmRSS:"
+if (line.rfind(linePrefix, 0) == 0) {
+  std::istringstream valuableLine(line.substr(linePrefix.length()));
+  uint64_t kByteValue;
+  valuableLine >> kByteValue;
+  return kByteValue * 1024;
+}
+  }
+
+  throw std::runtime_error("Could not get memory info for current process");
+#endif
+
+#ifdef __APPLE__
+  task_basic_info tInfo;
+  mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
+  if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, 
(task_info_t), ))
+throw std::runtime_error("Could not get memory info for current process");
+  return tInfo.resident_size;
+#endif
+
+#ifdef _WIN32
+  PROCESS_MEMORY_COUNTERS pmc;
+  if (!GetProcessMemoryInfo(GetCurrentProcess(), , sizeof(pmc)))
+throw std::runtime_error("Could not get memory info for current process");
+  return pmc.WorkingSetSize;
+#endif
+}

Review comment:
   ea9ef69 is about right but incorrectly indented





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

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #845: MINIFICPP-1277 - Create cross-platform API for querying memory usage

[GitBox]

szaszm commented on a change in pull request #845:
URL: https://github.com/apache/nifi-minifi-cpp/pull/845#discussion_r462864018



##
File path: libminifi/src/utils/OsUtils.cpp
##
@@ -154,6 +164,41 @@ std::string OsUtils::userIdToUsername(const std::string 
) {
   return name;
 }
 
+uint64_t OsUtils::getMemoryUsage() {
+#ifdef __linux__
+  static const std::string linePrefix = "VmRSS:";
+  std::ifstream statusFile("/proc/self/status");
+  std::string line;
+
+  while (std::getline(statusFile, line)) {
+// if line begins with "VmRSS:"
+if (line.rfind(linePrefix, 0) == 0) {
+  std::istringstream valuableLine(line.substr(linePrefix.length()));
+  uint64_t kByteValue;
+  valuableLine >> kByteValue;
+  return kByteValue * 1024;
+}
+  }
+
+  throw std::runtime_error("Could not get memory info for current process");
+#endif
+
+#ifdef __APPLE__
+  task_basic_info tInfo;
+  mach_msg_type_number_t tInfoCount = TASK_BASIC_INFO_COUNT;
+  if (KERN_SUCCESS != task_info(mach_task_self(), TASK_BASIC_INFO, 
(task_info_t), ))
+throw std::runtime_error("Could not get memory info for current process");
+  return tInfo.resident_size;
+#endif
+
+#ifdef _WIN32
+  PROCESS_MEMORY_COUNTERS pmc;
+  if (!GetProcessMemoryInfo(GetCurrentProcess(), , sizeof(pmc)))
+throw std::runtime_error("Could not get memory info for current process");
+  return pmc.WorkingSetSize;
+#endif
+}

Review comment:
   I suggest adding a last throw statement to act as a "catchall" on other 
platforms (e.g. *BSD, other unixes). 





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

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #845: MINIFICPP-1277 - Create cross-platform API for querying memory usage

[GitBox]

szaszm commented on a change in pull request #845:
URL: https://github.com/apache/nifi-minifi-cpp/pull/845#discussion_r459425267



##
File path: libminifi/src/utils/OsUtils.cpp
##
@@ -154,6 +160,80 @@ std::string OsUtils::userIdToUsername(const std::string 
) {
   return name;
 }
 
+unsigned long long OsUtils::getMemoryUsage() {
+#ifdef __linux__
+  long resPages;
+  long sharedPages;
+  {
+std::string ignore;
+std::ifstream ifs("/proc/self/statm");
+ifs >> ignore >> resPages >> sharedPages;
+  }
+
+  if (sharedPages > resPages) {
+throw std::range_error("Shared memory page count ("
+  + std::to_string(sharedPages)
+  + ") should not be larger than resident set size ("
+  + std::to_string(resPages)
+  + "), that includes it"
+);
+  }
+
+  const long ownPages = resPages - sharedPages;

Review comment:
   +1 for no `long`. If you need a 64 bit integer, why not make it explicit 
by using `int64_t`?
   
   https://google.github.io/styleguide/cppguide.html#Integer_Types
   
   Other issue: I believe `statm` doesn't make a difference between normal 
pages and hugepages, so while the number may be correct as the number of pages, 
multiplying it with the default page size might yield an incorrect "memory 
usage". 
https://stackoverflow.com/questions/1558402/memory-usage-of-current-process-in-c#comment93410400_7212248





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