- Compute the Path objects once, instead of parsing them for every metric.

- Use vespalib::string all over to avoid back and forth construction.

4 files changed, 144 insertions, 132 deletions

diff --git a/metrics/src/vespa/metrics/metric.h b/metrics/src/vespa/metrics/metric.h
index f254ec34074..10b74a2da22 100644
--- a/metrics/src/vespa/metrics/metric.h
+++ b/metrics/src/vespa/metrics/metric.h
@@ -13,7 +13,6 @@ struct AbstractValueMetric;
 class Metric;
 class MetricSet;
 class MetricSnapshot;
-class XmlWriterMetricVisitor;
 class MemoryConsumption;
 
 /** Implement class to visit metrics. */
@@ -104,7 +103,7 @@ private:
 class Metric : public vespalib::Printable
 {
 public:
-    using String = std::string;
+    using String = vespalib::string;
     using stringref = vespalib::stringref;
     using UP = std::unique_ptr<Metric>;
     using SP = std::shared_ptr<Metric>;
diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp
index 8ca74384af0..7fa13a06e7b 100644
--- a/metrics/src/vespa/metrics/metricmanager.cpp
+++ b/metrics/src/vespa/metrics/metricmanager.cpp
@@ -190,147 +190,160 @@ MetricManager::init(const config::ConfigUri & uri, FastOS_ThreadPool& pool,
 
 namespace {
 
-    struct Path {
-        vespalib::StringTokenizer _path;
+struct Path {
+    vespalib::StringTokenizer _path;
 
-        Path(vespalib::stringref fullpath) : _path(fullpath, ".") { }
+    Path(vespalib::stringref fullpath) : _path(fullpath, ".") { }
 
-        vespalib::string toString() const {
-            vespalib::asciistream ost;
-            ost << _path[0];
-            for (uint32_t i=1; i<path().size(); ++i) ost << "." << _path[i];
-            return ost.str();
-        }
+    vespalib::string toString() const {
+        vespalib::asciistream ost;
+        ost << _path[0];
+        for (uint32_t i=1; i<path().size(); ++i) ost << "." << _path[i];
+        return ost.str();
+    }
 
-        bool matchesPattern(const Path& p) const {
-            if (path().size() != p.path().size()) {
+    bool matchesPattern(const Path& p) const {
+        if (path().size() != p.path().size()) {
+            return false;
+        }
+        for (uint32_t i=0; i<p.path().size(); ++i) {
+            if (p._path[i] == "*") continue;
+            if (_path[i] != p._path[i]) {
                 return false;
             }
-            for (uint32_t i=0; i<p.path().size(); ++i) {
-                if (p._path[i] == "*") continue;
-                if (_path[i] != p._path[i]) {
-                    return false;
-                }
-            }
-            return true;
-        }
-        const vespalib::StringTokenizer::TokenList& path() const { 
-            return _path.getTokens();
         }
+        return true;
+    }
+    const vespalib::StringTokenizer::TokenList& path() const {
+        return _path.getTokens();
+    }
+};
+
+struct ConsumerMetricBuilder : public MetricVisitor {
+    const Config::Consumer& _consumer;
+    std::vector<Path> _added;
+    std::vector<Path> _removed;
+    MetricManager::ConsumerSpec _matchedMetrics;
+        // Keep a stack of matches to facilitate tree traversal.
+    struct Result {
+        bool tagAdded;
+        bool tagRemoved;
+        bool nameAdded;
+        bool nameRemoved;
+        uint32_t metricCount;
+
+        Result() : tagAdded(false), tagRemoved(false),
+                   nameAdded(false), nameRemoved(false), metricCount(0) {}
     };
+    std::list<Result> result;
 
-    struct ConsumerMetricBuilder : public MetricVisitor {
-        const Config::Consumer& _consumer;
-        MetricManager::ConsumerSpec _matchedMetrics;
-            // Keep a stack of matches to facilitate tree traversal.
-        struct Result {
-            bool tagAdded;
-            bool tagRemoved;
-            bool nameAdded;
-            bool nameRemoved;
-            uint32_t metricCount;
-
-            Result() : tagAdded(false), tagRemoved(false),
-                       nameAdded(false), nameRemoved(false), metricCount(0) {}
-        };
-        std::list<Result> result;
-
-        ConsumerMetricBuilder(const Config::Consumer& c) __attribute__((noinline));
-        ~ConsumerMetricBuilder() __attribute__((noinline));
-
-        bool tagAdded(const Metric& metric) {
-            for (const auto& s : _consumer.tags) {
-                if ((s == "*" && !metric.getTags().empty())
-                    || metric.hasTag(s))
-                {
-                    return true;
-                }
+    ConsumerMetricBuilder(const Config::Consumer& c) __attribute__((noinline));
+    ~ConsumerMetricBuilder() __attribute__((noinline));
+
+    bool tagAdded(const Metric& metric) {
+        for (const auto& s : _consumer.tags) {
+            if ((s == "*" && !metric.getTags().empty())
+                || metric.hasTag(s))
+            {
+                return true;
             }
-            return false;
         }
-        bool tagRemoved(const Metric& metric) {
-            for (const auto& s : _consumer.removedtags) {
-                if ((s == "*" && !metric.getTags().empty())
-                    || metric.hasTag(s))
-                {
-                    return true;
-                }
+        return false;
+    }
+    bool tagRemoved(const Metric& metric) {
+        for (const auto& s : _consumer.removedtags) {
+            if ((s == "*" && !metric.getTags().empty())
+                || metric.hasTag(s))
+            {
+                return true;
             }
-            return false;
         }
-        bool nameAdded(const Metric& metric) {
-            Metric::String path(metric.getPath());
-            Path mpath(path);
-            for (const auto& s : _consumer.addedmetrics) {
-                if (mpath.matchesPattern(Path(s))) {
-                    return true;
-                }
+        return false;
+    }
+    bool nameAdded(const Path& mpath) {
+        for (const auto& p : _added) {
+            if (mpath.matchesPattern(p)) {
+                return true;
             }
-            return false;
         }
-        bool nameRemoved(const Metric& metric) {
-            Metric::String path(metric.getPath());
-            Path mpath(path);
-            for (const auto& s : _consumer.removedmetrics) {
-                if (mpath.matchesPattern(Path(s))) {
-                    return true;
-                }
+        return false;
+    }
+    bool nameRemoved(const Path & mpath) {
+        for (const auto& p : _removed) {
+            if (mpath.matchesPattern(p)) {
+                return true;
             }
-            return false;
         }
+        return false;
+    }
 
-        bool visitMetricSet(const MetricSet& metricSet,
-                                    bool autoGenerated) override
-        {
-            (void) autoGenerated;
-            result.push_back(Result());
-                // If current metric match anything, use that.
-                // Otherwise, copy from parent
-            if (nameRemoved(metricSet)) {
-                result.back().nameRemoved = true;
-            } else if (nameAdded(metricSet)) {
-                result.back().nameAdded = true;
-            } else if (tagRemoved(metricSet)) {
-                result.back().tagRemoved = true;
-            } else if (tagAdded(metricSet)) {
-                result.back().tagAdded = true;
-            } else if (result.size() > 1) {
-                result.back() = *++result.rbegin();
-                result.back().metricCount = 0;
-            }
-            return true;
+    bool visitMetricSet(const MetricSet& metricSet, bool autoGenerated) override
+    {
+        (void) autoGenerated;
+        result.push_back(Result());
+        // If current metric match anything, use that.
+        // Otherwise, copy from parent
+        vespalib::string fullName = metricSet.getPath();
+        Path path(fullName);
+        if (nameRemoved(path)) {
+            result.back().nameRemoved = true;
+        } else if (nameAdded(path)) {
+            result.back().nameAdded = true;
+        } else if (tagRemoved(metricSet)) {
+            result.back().tagRemoved = true;
+        } else if (tagAdded(metricSet)) {
+            result.back().tagAdded = true;
+        } else if (result.size() > 1) {
+            result.back() = *++result.rbegin();
+            result.back().metricCount = 0;
         }
-        void doneVisitingMetricSet(const MetricSet& metricSet) override {
-            if (result.back().metricCount > 0 && result.size() != 1) {
-                LOG(spam, "Adding metricset %s", metricSet.getPath().c_str());
-                _matchedMetrics.includedMetrics.insert(metricSet.getPath());
-            }
-            result.pop_back();
+        return true;
+    }
+    void doneVisitingMetricSet(const MetricSet& metricSet) override {
+        if (result.back().metricCount > 0 && result.size() != 1) {
+            LOG(spam, "Adding metricset %s", metricSet.getPath().c_str());
+            _matchedMetrics.includedMetrics.insert(metricSet.getPath());
         }
-        bool visitMetric(const Metric& metric, bool autoGenerated) override {
-            (void) autoGenerated;
-            if (result.back().nameRemoved || nameRemoved(metric)) return true;
-            bool nAdded = (result.back().nameAdded || nameAdded(metric));
-            if (!nAdded && (result.back().tagRemoved || tagRemoved(metric))) {
-                return true;
-            }
-            if (nAdded || result.back().tagAdded || tagAdded(metric)) {
-                _matchedMetrics.includedMetrics.insert(metric.getPath());
-                LOG(spam, "Adding metric %s", metric.getPath().c_str());
-                for (Result & r : result) {
-                    ++r.metricCount;
-                }
-            }
+        result.pop_back();
+    }
+    bool visitMetric(const Metric& metric, bool autoGenerated) override {
+        (void) autoGenerated;
+        vespalib::string fullName = metric.getPath();
+        Path path(fullName);
+        if (result.back().nameRemoved || nameRemoved(path)) return true;
+        bool nAdded = (result.back().nameAdded || nameAdded(path));
+        if (!nAdded && (result.back().tagRemoved || tagRemoved(metric))) {
             return true;
         }
+        if (nAdded || result.back().tagAdded || tagAdded(metric)) {
+            _matchedMetrics.includedMetrics.insert(fullName);
+            LOG(spam, "Adding metric %s", fullName.c_str());
+            for (Result & r : result) {
+                ++r.metricCount;
+            }
+        }
+        return true;
+    }
 
-    };
-    ConsumerMetricBuilder::ConsumerMetricBuilder(const Config::Consumer& c)
-        : _consumer(c), _matchedMetrics()
-    {
-        LOG(spam, "Adding metrics for consumer %s", c.name.c_str());
+};
+
+ConsumerMetricBuilder::ConsumerMetricBuilder(const Config::Consumer& c)
+    : _consumer(c),
+      _added(),
+      _removed(),
+      _matchedMetrics()
+{
+    _added.reserve(_consumer.addedmetrics.size());
+    for (const auto& s : _consumer.addedmetrics) {
+        _added.emplace_back(s);
+    }
+    _removed.reserve(_consumer.removedmetrics.size());
+    for (const auto& s : _consumer.removedmetrics) {
+        _removed.emplace_back(s);
     }
-    ConsumerMetricBuilder::~ConsumerMetricBuilder() = default;
+    LOG(spam, "Adding metrics for consumer %s", c.name.c_str());
+}
+ConsumerMetricBuilder::~ConsumerMetricBuilder() = default;
 
 }
 
@@ -352,8 +365,8 @@ void
 MetricManager::handleMetricsAltered(const MetricLockGuard & guard)
 {
     (void) guard;
-    if (_config.get() == NULL) {
-        LOG(info, "_config is NULL -> very odd indeed.");
+    if ( ! _config ) {
+        LOG(info, "_config is not set -> very odd indeed.");
         return;
     }
     if (_consumerConfig.empty()) {
@@ -367,7 +380,7 @@ MetricManager::handleMetricsAltered(const MetricLockGuard & guard)
     for (const auto & consumer : _config->consumer) {
         ConsumerMetricBuilder consumerMetricBuilder(consumer);
         _activeMetrics.getMetrics().visit(consumerMetricBuilder);
-        configMap[consumer.name] = ConsumerSpec::SP(new ConsumerSpec(std::move(consumerMetricBuilder._matchedMetrics)));
+        configMap[consumer.name] = std::make_shared<ConsumerSpec>(std::move(consumerMetricBuilder._matchedMetrics));
     }
     LOG(debug, "Recreating snapshots to include altered metrics");
     _totalMetrics->recreateSnapshot(_activeMetrics.getMetrics(),
diff --git a/metrics/src/vespa/metrics/metricset.cpp b/metrics/src/vespa/metrics/metricset.cpp
index 2916b32eb56..e994015d487 100644
--- a/metrics/src/vespa/metrics/metricset.cpp
+++ b/metrics/src/vespa/metrics/metricset.cpp
@@ -51,7 +51,7 @@ MetricSet::clone(std::vector<Metric::UP> &ownerList, CopyType type,
 
 
 const Metric*
-MetricSet::getMetricInternal(const String& name) const
+MetricSet::getMetricInternal(stringref name) const
 {
     for (const Metric* metric : _metricOrder) {
         if (metric->getMangledName() == name) {
@@ -71,14 +71,14 @@ double MetricSet::getDoubleValue(stringref) const {
 }
 
 const Metric*
-MetricSet::getMetric(const String& name) const
+MetricSet::getMetric(stringref name) const
 {
-    std::string::size_type pos = name.find('.');
-    if (pos == std::string::npos) {
+    vespalib::string::size_type pos = name.find('.');
+    if (pos == vespalib::string::npos) {
         return getMetricInternal(name);
     } else {
-        std::string child(name.substr(0, pos));
-        std::string rest(name.substr(pos+1));
+        stringref child(name.substr(0, pos));
+        stringref rest(name.substr(pos+1));
         const Metric* m(getMetricInternal(child));
         if (m == 0) return 0;
         if (!m->isMetricSet()) {
diff --git a/metrics/src/vespa/metrics/metricset.h b/metrics/src/vespa/metrics/metricset.h
index 66225b8f868..84b5eb8c848 100644
--- a/metrics/src/vespa/metrics/metricset.h
+++ b/metrics/src/vespa/metrics/metricset.h
@@ -57,8 +57,8 @@ public:
     int64_t getLongValue(stringref id) const override;
     double getDoubleValue(stringref id) const override;
 
-    const Metric* getMetric(const String& name) const;
-    Metric* getMetric(const String& name) {
+    const Metric* getMetric(stringref name) const;
+    Metric* getMetric(stringref name) {
         return const_cast<Metric*>(
                 const_cast<const MetricSet*>(this)->getMetric(name));
     }
@@ -81,7 +81,7 @@ private:
     MetricSet& operator=(const MetricSet&);
 
     void tagRegistrationAltered();
-    const Metric* getMetricInternal(const String& name) const;
+    const Metric* getMetricInternal(stringref name) const;
 
     virtual void addTo(Metric&, std::vector<Metric::UP> *ownerList) const;
 };