Fix .ninja parse time reported by `-d stats`.
Because Parser::Load() is called recursively during Ninja
manifest parsing, the call to METRIC_RECORD() in this function
used to over-count the total parsing time (for example, by a
factor of 2 for the Fuchsia build).
This fixes the problem by introducing a new RECORD_METRIC_IF()
macro, which only records anything if a given condition is true.
This ensures that metric collection only starts and stops with
the outer Parser::Load() call, and none of its recursive
sub-calls.
The effect on the output of `-d stats`is, for a Fuchsia build
plan where `ninja -d stats nothing` takes a bit more than 5s:
BEFORE:
metric count avg (us) total (ms)
.ninja parse 27304 372.6 10172.2
AFTER:
metric count avg (us) total (ms)
.ninja parse 1 4165297.0 4165.3
Note that |count| went to 1, since there is only one top-level
Parser::Load() operation in this build. It would be more if
dyndeps files were loaded, which does not happen in this
build plan.
Upstream-Pull-Request: https://github.com/ninja-build/ninja/pull/2292
Original-Change-Id: Id68874d56fa6899f77d0c1872cc8b7701fe8cf6e
Change-Id: I8dc2d8a86822759b8a52072d955f8830097f6ab6
diff --git a/src/metrics.h b/src/metrics.h
index c9ba236..937d905 100644
--- a/src/metrics.h
+++ b/src/metrics.h
@@ -85,6 +85,13 @@
g_metrics ? g_metrics->NewMetric(name) : NULL; \
ScopedMetric metrics_h_scoped(metrics_h_metric);
+/// A variant of METRIC_RECORD that doesn't record anything if |condition|
+/// is false.
+#define METRIC_RECORD_IF(name, condition) \
+ static Metric* metrics_h_metric = \
+ g_metrics ? g_metrics->NewMetric(name) : NULL; \
+ ScopedMetric metrics_h_scoped((condition) ? metrics_h_metric : NULL);
+
extern Metrics* g_metrics;
#endif // NINJA_METRICS_H_
diff --git a/src/parser.cc b/src/parser.cc
index 5f303c5..139a347 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -20,7 +20,10 @@
using namespace std;
bool Parser::Load(const string& filename, string* err, Lexer* parent) {
- METRIC_RECORD(".ninja parse");
+ // If |parent| is not NULL, metrics collection has been started by a parent
+ // Parser::Load() in our call stack. Do not start a new one here to avoid
+ // over-counting parsing times.
+ METRIC_RECORD_IF(".ninja parse", parent == NULL);
string contents;
string read_err;
if (file_reader_->ReadFile(filename, &contents, &read_err) !=
