Merge topic 'instrumentation-fix'

e01d12c14f instrumentation: Prevent unnecessary query loading
f26f127183 instrumentation: Reuse single cmsys::SystemInformation

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !10863

Source/cmInstrumentation.cxx

@@ -3,11 +3,11 @@
 #include <chrono>
 #include <ctime>
 #include <iomanip>
-#include <memory>
 #include <set>
 #include <sstream>
 #include <utility>
 
+#include <cm/memory>
 #include <cm/optional>
 
 #include <cm3p/json/writer.h>
@@ -27,7 +27,10 @@
 #include "cmUVProcessChain.h"
 #include "cmValue.h"
 
-cmInstrumentation::cmInstrumentation(std::string const& binary_dir)
+using LoadQueriesAfter = cmInstrumentation::LoadQueriesAfter;
+
+cmInstrumentation::cmInstrumentation(std::string const& binary_dir,
+                                     LoadQueriesAfter loadQueries)
 {
   std::string const uuid =
     cmExperimental::DataForFeature(cmExperimental::Feature::Instrumentation)
@@ -40,7 +43,9 @@ cmInstrumentation::cmInstrumentation(std::string const& binary_dir)
     this->userTimingDirv1 =
       cmStrCat(configDir.value(), "/instrumentation-", uuid, "/v1");
   }
-  this->LoadQueries();
+  if (loadQueries == LoadQueriesAfter::Yes) {
+    this->LoadQueries();
+  }
 }
 
 void cmInstrumentation::LoadQueries()
@@ -126,6 +131,14 @@ void cmInstrumentation::LoadQueries()
   }
 }
 
+cmsys::SystemInformation& cmInstrumentation::GetSystemInformation()
+{
+  if (!this->systemInformation) {
+    this->systemInformation = cm::make_unique<cmsys::SystemInformation>();
+  }
+  return *this->systemInformation;
+}
+
 bool cmInstrumentation::ReadJSONQueries(std::string const& directory)
 {
   cmsys::Directory d;
@@ -220,8 +233,7 @@ bool cmInstrumentation::HasPreOrPostBuildHook() const
 int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
 {
   // Don't run collection if hook is disabled
-  if (hook != cmInstrumentationQuery::Hook::Manual &&
-      this->hooks.find(hook) == this->hooks.end()) {
+  if (hook != cmInstrumentationQuery::Hook::Manual && !this->HasHook(hook)) {
     return 0;
   }
 
@@ -309,36 +321,38 @@ int cmInstrumentation::CollectTimingData(cmInstrumentationQuery::Hook hook)
 void cmInstrumentation::InsertDynamicSystemInformation(
   Json::Value& root, std::string const& prefix)
 {
-  cmsys::SystemInformation info;
   Json::Value data;
-  info.RunCPUCheck();
-  info.RunMemoryCheck();
+  double memory;
+  double load;
+  this->GetDynamicSystemInformation(memory, load);
   if (!root.isMember("dynamicSystemInformation")) {
     root["dynamicSystemInformation"] = Json::objectValue;
   }
   root["dynamicSystemInformation"][cmStrCat(prefix, "HostMemoryUsed")] =
-    (double)info.GetHostMemoryUsed();
-  root["dynamicSystemInformation"][cmStrCat(prefix, "CPULoadAverage")] =
-    info.GetLoadAverage();
+    memory;
+  root["dynamicSystemInformation"][cmStrCat(prefix, "CPULoadAverage")] = load;
 }
 
 void cmInstrumentation::GetDynamicSystemInformation(double& memory,
                                                     double& load)
 {
-  cmsys::SystemInformation info;
-  Json::Value data;
-  info.RunCPUCheck();
-  info.RunMemoryCheck();
+  cmsys::SystemInformation& info = this->GetSystemInformation();
+  if (!this->ranSystemChecks) {
+    info.RunCPUCheck();
+    info.RunMemoryCheck();
+    this->ranSystemChecks = true;
+  }
   memory = (double)info.GetHostMemoryUsed();
   load = info.GetLoadAverage();
 }
 
 void cmInstrumentation::InsertStaticSystemInformation(Json::Value& root)
 {
-  cmsys::SystemInformation info;
-  info.RunCPUCheck();
-  info.RunOSCheck();
-  info.RunMemoryCheck();
+  cmsys::SystemInformation& info = this->GetSystemInformation();
+  if (!this->ranOSCheck) {
+    info.RunOSCheck();
+    this->ranOSCheck = true;
+  }
   Json::Value infoRoot;
   infoRoot["familyId"] = info.GetFamilyID();
   infoRoot["hostname"] = info.GetHostname();
@@ -418,7 +432,7 @@ std::string cmInstrumentation::InstrumentTest(
     this->InsertDynamicSystemInformation(root, "after");
   }
 
-  cmsys::SystemInformation info;
+  cmsys::SystemInformation& info = this->GetSystemInformation();
   std::string file_name = cmStrCat(
     "test-",
     this->ComputeSuffixHash(cmStrCat(command_str, info.GetProcessId())),
@@ -440,12 +454,12 @@ int cmInstrumentation::InstrumentCommand(
   std::function<int()> const& callback,
   cm::optional<std::map<std::string, std::string>> options,
   cm::optional<std::map<std::string, std::string>> arrayOptions,
-  bool reloadQueriesAfterCommand)
+  LoadQueriesAfter reloadQueriesAfterCommand)
 {
 
   // Always begin gathering data for configure in case cmake_instrumentation
   // command creates a query
-  if (!this->hasQuery && !reloadQueriesAfterCommand) {
+  if (!this->hasQuery && reloadQueriesAfterCommand == LoadQueriesAfter::No) {
     return callback();
   }
 
@@ -467,7 +481,7 @@ int cmInstrumentation::InstrumentCommand(
   if (this->HasQuery(
         cmInstrumentationQuery::Query::DynamicSystemInformation)) {
     this->InsertDynamicSystemInformation(root, "before");
-  } else if (reloadQueriesAfterCommand) {
+  } else if (reloadQueriesAfterCommand == LoadQueriesAfter::Yes) {
     this->GetDynamicSystemInformation(preConfigureMemory, preConfigureLoad);
   }
 
@@ -476,9 +490,9 @@ int cmInstrumentation::InstrumentCommand(
   root["result"] = ret;
 
   // Exit early if configure didn't generate a query
-  if (reloadQueriesAfterCommand) {
+  if (reloadQueriesAfterCommand == LoadQueriesAfter::Yes) {
     this->LoadQueries();
-    if (!this->hasQuery) {
+    if (!this->HasQuery()) {
       return ret;
     }
     if (this->HasQuery(
@@ -540,7 +554,7 @@ int cmInstrumentation::InstrumentCommand(
   root["workingDir"] = cmSystemTools::GetLogicalWorkingDirectory();
 
   // Write Json
-  cmsys::SystemInformation info;
+  cmsys::SystemInformation& info = this->GetSystemInformation();
   std::string const& file_name = cmStrCat(
     command_type, '-',
     this->ComputeSuffixHash(cmStrCat(command_str, info.GetProcessId())),
@@ -635,7 +649,7 @@ int cmInstrumentation::CollectTimingAfterBuild(int ppid)
   };
   int ret = this->InstrumentCommand(
     "build", {}, [waitForBuild]() { return waitForBuild(); }, cm::nullopt,
-    cm::nullopt, false);
+    cm::nullopt, LoadQueriesAfter::No);
   this->CollectTimingData(cmInstrumentationQuery::Hook::PostBuild);
   return ret;
 }

Source/cmInstrumentation.h

@@ -11,9 +11,13 @@
 #include <string>
 #include <vector>
 
+#include <cm/memory>
 #include <cm/optional>
 
 #include <cm3p/json/value.h>
+#ifndef CMAKE_BOOTSTRAP
+#  include <cmsys/SystemInformation.hxx>
+#endif
 #include <stdint.h>
 
 #include "cmInstrumentationQuery.h"
@@ -21,14 +25,21 @@
 class cmInstrumentation
 {
 public:
-  cmInstrumentation(std::string const& binary_dir);
+  enum class LoadQueriesAfter
+  {
+    Yes,
+    No
+  };
+  cmInstrumentation(std::string const& binary_dir,
+                    LoadQueriesAfter loadQueries = LoadQueriesAfter::Yes);
+  void LoadQueries();
   int InstrumentCommand(
     std::string command_type, std::vector<std::string> const& command,
     std::function<int()> const& callback,
     cm::optional<std::map<std::string, std::string>> options = cm::nullopt,
     cm::optional<std::map<std::string, std::string>> arrayOptions =
       cm::nullopt,
-    bool reloadQueriesAfterCommand = false);
+    LoadQueriesAfter reloadQueriesAfterCommand = LoadQueriesAfter::No);
   std::string InstrumentTest(std::string const& name,
                              std::string const& command,
                              std::vector<std::string> const& args,
@@ -37,7 +48,6 @@ public:
                              std::chrono::system_clock::time_point systemStart,
                              std::string config);
   void GetPreTestStats();
-  void LoadQueries();
   bool HasQuery() const;
   bool HasQuery(cmInstrumentationQuery::Query) const;
   bool HasHook(cmInstrumentationQuery::Hook) const;
@@ -60,13 +70,13 @@ private:
   void WriteInstrumentationJson(Json::Value& index,
                                 std::string const& directory,
                                 std::string const& file_name);
-  static void InsertStaticSystemInformation(Json::Value& index);
-  static void GetDynamicSystemInformation(double& memory, double& load);
-  static void InsertDynamicSystemInformation(Json::Value& index,
-                                             std::string const& instant);
-  static void InsertTimingData(
-    Json::Value& root, std::chrono::steady_clock::time_point steadyStart,
-    std::chrono::system_clock::time_point systemStart);
+  void InsertStaticSystemInformation(Json::Value& index);
+  void GetDynamicSystemInformation(double& memory, double& load);
+  void InsertDynamicSystemInformation(Json::Value& index,
+                                      std::string const& instant);
+  void InsertTimingData(Json::Value& root,
+                        std::chrono::steady_clock::time_point steadyStart,
+                        std::chrono::system_clock::time_point systemStart);
   bool HasQueryFile(std::string const& file);
   static std::string GetCommandStr(std::vector<std::string> const& args);
   static std::string ComputeSuffixHash(std::string const& command_str);
@@ -85,4 +95,10 @@ private:
   Json::Value preTestStats;
   std::string errorMsg;
   bool hasQuery = false;
+  bool ranSystemChecks = false;
+  bool ranOSCheck = false;
+#ifndef CMAKE_BOOTSTRAP
+  std::unique_ptr<cmsys::SystemInformation> systemInformation;
+  cmsys::SystemInformation& GetSystemInformation();
+#endif
 };

Source/cmake.cxx

@@ -2633,16 +2633,18 @@ int cmake::ActualConfigure()
   this->FileAPI = cm::make_unique<cmFileAPI>(this);
   this->FileAPI->ReadQueries();
 
+  this->Instrumentation = cm::make_unique<cmInstrumentation>(
+    this->State->GetBinaryDirectory(),
+    cmInstrumentation::LoadQueriesAfter::No);
+  this->Instrumentation->ClearGeneratedQueries();
+
   if (!this->GetIsInTryCompile()) {
     this->TruncateOutputLog("CMakeConfigureLog.yaml");
     this->ConfigureLog = cm::make_unique<cmConfigureLog>(
       cmStrCat(this->GetHomeOutputDirectory(), "/CMakeFiles"_s),
       this->FileAPI->GetConfigureLogVersions());
+    this->Instrumentation->LoadQueries();
   }
-
-  this->Instrumentation =
-    cm::make_unique<cmInstrumentation>(this->State->GetBinaryDirectory());
-  this->Instrumentation->ClearGeneratedQueries();
 #endif
 
   // actually do the configure
@@ -2657,7 +2659,9 @@ int cmake::ActualConfigure()
   };
   int ret = this->Instrumentation->InstrumentCommand(
     "configure", this->cmdArgs, [doConfigure]() { return doConfigure(); },
-    cm::nullopt, cm::nullopt, true);
+    cm::nullopt, cm::nullopt,
+    this->GetIsInTryCompile() ? cmInstrumentation::LoadQueriesAfter::No
+                              : cmInstrumentation::LoadQueriesAfter::Yes);
   if (ret != 0) {
     return ret;
   }
@@ -2700,7 +2704,6 @@ int cmake::ActualConfigure()
   }
   // Setup launchers for instrumentation
 #if !defined(CMAKE_BOOTSTRAP)
-  this->Instrumentation->LoadQueries();
   if (this->Instrumentation->HasQuery()) {
     std::string launcher;
     if (mf->IsOn("CTEST_USE_LAUNCHERS")) {
@@ -3063,7 +3066,6 @@ int cmake::Generate()
     return 0;
   };
 
-  this->Instrumentation->LoadQueries();
   int ret = this->Instrumentation->InstrumentCommand(
     "generate", this->cmdArgs, [doGenerate]() { return doGenerate(); });
   if (ret != 0) {

Tests/RunCMake/Instrumentation/RunCMakeTest.cmake

@@ -6,7 +6,7 @@ function(instrument test)
   set(config "${CMAKE_CURRENT_LIST_DIR}/config")
   set(ENV{CMAKE_CONFIG_DIR} ${config})
   cmake_parse_arguments(ARGS
-    "BUILD;BUILD_MAKE_PROGRAM;INSTALL;TEST;COPY_QUERIES;NO_WARN;STATIC_QUERY;DYNAMIC_QUERY;INSTALL_PARALLEL;MANUAL_HOOK"
+    "BUILD;BUILD_MAKE_PROGRAM;INSTALL;TEST;COPY_QUERIES;COPY_QUERIES_GENERATED;NO_WARN;STATIC_QUERY;DYNAMIC_QUERY;INSTALL_PARALLEL;MANUAL_HOOK"
     "CHECK_SCRIPT;CONFIGURE_ARG" "" ${ARGN})
   set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/${test})
   set(uuid "a37d1069-1972-4901-b9c9-f194aaf2b6e0")
@@ -34,14 +34,18 @@ function(instrument test)
     list(APPEND ARGS_CONFIGURE_ARG "-DINSTRUMENT_COMMAND_FILE=${cmake_file}")
   endif()
 
-  # Configure generated query files to compare CMake output
+  set(copy_loc ${RunCMake_TEST_BINARY_DIR}/query)
+  if (ARGS_COPY_QUERIES_GENERATED)
+    set(ARGS_COPY_QUERIES TRUE)
+    set(copy_loc ${v1}/query/generated) # Copied files here should be cleared on configure
+  endif()
   if (ARGS_COPY_QUERIES)
-    file(MAKE_DIRECTORY ${RunCMake_TEST_BINARY_DIR}/query)
+    file(MAKE_DIRECTORY ${copy_loc})
     set(generated_queries "0;1;2")
     foreach(n IN LISTS generated_queries)
       configure_file(
         "${query_dir}/generated/query-${n}.json.in"
-        "${RunCMake_TEST_BINARY_DIR}/query/query-${n}.json"
+        "${copy_loc}/query-${n}.json"
       )
     endforeach()
   endif()
@@ -118,6 +122,10 @@ instrument(cmake-command-bad-arg NO_WARN)
 instrument(cmake-command-parallel-install
   BUILD INSTALL TEST NO_WARN INSTALL_PARALLEL DYNAMIC_QUERY
   CHECK_SCRIPT check-data-dir.cmake)
+instrument(cmake-command-resets-generated NO_WARN
+  COPY_QUERIES_GENERATED
+  CHECK_SCRIPT check-data-dir.cmake
+)
 
 # FIXME(#26668) This does not work on Windows
 if (UNIX)

Tests/RunCMake/Instrumentation/query/cmake-command-resets-generated.cmake

@@ -0,0 +1,4 @@
+cmake_instrumentation(
+  API_VERSION 1
+  DATA_VERSION 1
+)