Merge #18662: test: Replace gArgs with local argsman in bench

faf989f util: Document why ArgsManager (con/de)structor is not inline (MarcoFalke)
fae00a7 bench: Remove unused argsman.ClearArgs (MarcoFalke)
fa46aeb scripted-diff: Replace gArgs with local argsman in bench (MarcoFalke)
fa2bc41 tools: Add unused argsman to bench_bitcoin (MarcoFalke)

Pull request description:

  All utilities use the same gArgs global that the node uses. This is generally fine and does not lead to issues unless a bench test is going to spin up a NodeContext via the TestingSetup. In that case the two uses of gArgs conflict and currently it needs to be cleared:

  https://github.com/bitcoin/bitcoin/blob/544709763e1f45148d1926831e07ff03487673ee/src/bench/bench_bitcoin.cpp#L76

  One solution would be to do nothing, because the current code works with that workaround. Another solution would be to not use the same global in all binaries.

ACKs for top commit:
  promag:
    ACK faf989f.
  ryanofsky:
    Code review ACK faf989f. Just new commit added restoring forward declaration

Tree-SHA512: 8ee4b28eee294d41c002f801fa844b0c23c919a3061f5109638701db0947b3b0ea28caa7311ae5f126fc660648bbaa0890853e6b06bdc5868692f52ba8c05f66

Author: MarcoFalke

src/bench/bench_bitcoin.cpp

@@ -17,39 +17,40 @@ static const char* DEFAULT_PLOT_PLOTLYURL = "https://cdn.plot.ly/plotly-latest.m
 static const int64_t DEFAULT_PLOT_WIDTH = 1024;
 static const int64_t DEFAULT_PLOT_HEIGHT = 768;
 
-static void SetupBenchArgs()
+static void SetupBenchArgs(ArgsManager& argsman)
 {
-    SetupHelpOptions(gArgs);
-
-    gArgs.AddArg("-list", "List benchmarks without executing them. Can be combined with -scaling and -filter", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-evals=<n>", strprintf("Number of measurement evaluations to perform. (default: %u)", DEFAULT_BENCH_EVALUATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-filter=<regex>", strprintf("Regular expression filter to select benchmark by name (default: %s)", DEFAULT_BENCH_FILTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-scaling=<n>", strprintf("Scaling factor for benchmark's runtime (default: %u)", DEFAULT_BENCH_SCALING), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-printer=(console|plot)", strprintf("Choose printer format. console: print data to console. plot: Print results as HTML graph (default: %s)", DEFAULT_BENCH_PRINTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-plot-plotlyurl=<uri>", strprintf("URL to use for plotly.js (default: %s)", DEFAULT_PLOT_PLOTLYURL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-plot-width=<x>", strprintf("Plot width in pixel (default: %u)", DEFAULT_PLOT_WIDTH), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
-    gArgs.AddArg("-plot-height=<x>", strprintf("Plot height in pixel (default: %u)", DEFAULT_PLOT_HEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    SetupHelpOptions(argsman);
+
+    argsman.AddArg("-list", "List benchmarks without executing them. Can be combined with -scaling and -filter", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-evals=<n>", strprintf("Number of measurement evaluations to perform. (default: %u)", DEFAULT_BENCH_EVALUATIONS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-filter=<regex>", strprintf("Regular expression filter to select benchmark by name (default: %s)", DEFAULT_BENCH_FILTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-scaling=<n>", strprintf("Scaling factor for benchmark's runtime (default: %u)", DEFAULT_BENCH_SCALING), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-printer=(console|plot)", strprintf("Choose printer format. console: print data to console. plot: Print results as HTML graph (default: %s)", DEFAULT_BENCH_PRINTER), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-plot-plotlyurl=<uri>", strprintf("URL to use for plotly.js (default: %s)", DEFAULT_PLOT_PLOTLYURL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-plot-width=<x>", strprintf("Plot width in pixel (default: %u)", DEFAULT_PLOT_WIDTH), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-plot-height=<x>", strprintf("Plot height in pixel (default: %u)", DEFAULT_PLOT_HEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 }
 
 int main(int argc, char** argv)
 {
-    SetupBenchArgs();
+    ArgsManager argsman;
+    SetupBenchArgs(argsman);
     std::string error;
-    if (!gArgs.ParseParameters(argc, argv, error)) {
+    if (!argsman.ParseParameters(argc, argv, error)) {
         tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error);
         return EXIT_FAILURE;
     }
 
-    if (HelpRequested(gArgs)) {
-        std::cout << gArgs.GetHelpMessage();
+    if (HelpRequested(argsman)) {
+        std::cout << argsman.GetHelpMessage();
 
         return EXIT_SUCCESS;
     }
 
-    int64_t evaluations = gArgs.GetArg("-evals", DEFAULT_BENCH_EVALUATIONS);
-    std::string regex_filter = gArgs.GetArg("-filter", DEFAULT_BENCH_FILTER);
-    std::string scaling_str = gArgs.GetArg("-scaling", DEFAULT_BENCH_SCALING);
-    bool is_list_only = gArgs.GetBoolArg("-list", false);
+    int64_t evaluations = argsman.GetArg("-evals", DEFAULT_BENCH_EVALUATIONS);
+    std::string regex_filter = argsman.GetArg("-filter", DEFAULT_BENCH_FILTER);
+    std::string scaling_str = argsman.GetArg("-scaling", DEFAULT_BENCH_SCALING);
+    bool is_list_only = argsman.GetBoolArg("-list", false);
 
     if (evaluations == 0) {
         return EXIT_SUCCESS;
@@ -65,16 +66,14 @@ int main(int argc, char** argv)
     }
 
     std::unique_ptr<benchmark::Printer> printer = MakeUnique<benchmark::ConsolePrinter>();
-    std::string printer_arg = gArgs.GetArg("-printer", DEFAULT_BENCH_PRINTER);
+    std::string printer_arg = argsman.GetArg("-printer", DEFAULT_BENCH_PRINTER);
     if ("plot" == printer_arg) {
         printer.reset(new benchmark::PlotlyPrinter(
-            gArgs.GetArg("-plot-plotlyurl", DEFAULT_PLOT_PLOTLYURL),
-            gArgs.GetArg("-plot-width", DEFAULT_PLOT_WIDTH),
-            gArgs.GetArg("-plot-height", DEFAULT_PLOT_HEIGHT)));
+            argsman.GetArg("-plot-plotlyurl", DEFAULT_PLOT_PLOTLYURL),
+            argsman.GetArg("-plot-width", DEFAULT_PLOT_WIDTH),
+            argsman.GetArg("-plot-height", DEFAULT_PLOT_HEIGHT)));
     }
 
-    gArgs.ClearArgs(); // gArgs no longer needed. Clear it here to avoid interactions with the testing setup in the benches
-
     benchmark::BenchRunner::RunAll(*printer, evaluations, scaling_factor, regex_filter, is_list_only);
 
     return EXIT_SUCCESS;

src/util/system.cpp

@@ -226,10 +226,11 @@ static bool CheckValid(const std::string& key, const util::SettingsValue& val, u
     return true;
 }
 
-ArgsManager::ArgsManager()
-{
-    // nothing to do
-}
+// Define default constructor and destructor that are not inline, so code instantiating this class doesn't need to
+// #include class definitions for all members.
+// For example, m_settings has an internal dependency on univalue.
+ArgsManager::ArgsManager() {}
+ArgsManager::~ArgsManager() {}
 
 const std::set<std::string> ArgsManager::GetUnsuitableSectionOnlyArgs() const
 {

src/util/system.h

@@ -192,6 +192,7 @@ class ArgsManager
 
 public:
     ArgsManager();
+    ~ArgsManager();
 
     /**
      * Select the network in use