diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2020-11-23 17:50:41 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-11-23 17:50:41 +0100 |
commit | 66445f857b81b051c1572409ce19d032cb03024c (patch) | |
tree | c64f7eb471229e0843c623c37cbf8e3fc69e8849 | |
parent | bb001b8e430b0d85ec8a463d92940c843f0facea (diff) | |
parent | 7d53ab1b5a5e607b0e3023755b5ae4048cbb2c09 (diff) |
Merge pull request #15385 from vespa-engine/balder/reorder-for-smaller-footprint
Balder/reorder for smaller footprint
42 files changed, 304 insertions, 302 deletions
diff --git a/documentapi/src/vespa/documentapi/loadtypes/loadtypeset.cpp b/documentapi/src/vespa/documentapi/loadtypes/loadtypeset.cpp index cde15776b62..42aa79332db 100644 --- a/documentapi/src/vespa/documentapi/loadtypes/loadtypeset.cpp +++ b/documentapi/src/vespa/documentapi/loadtypes/loadtypeset.cpp @@ -38,7 +38,7 @@ LoadTypeSet::LoadTypeSet(const LoadTypeConfig& config) configure(config); } -LoadTypeSet::~LoadTypeSet() { } +LoadTypeSet::~LoadTypeSet() = default; void LoadTypeSet::addLoadType(uint32_t id, const string& name, Priority::Value priority) { diff --git a/messagebus/src/tests/simpleprotocol/simpleprotocol.cpp b/messagebus/src/tests/simpleprotocol/simpleprotocol.cpp index 715ef597767..bc4fc80abad 100644 --- a/messagebus/src/tests/simpleprotocol/simpleprotocol.cpp +++ b/messagebus/src/tests/simpleprotocol/simpleprotocol.cpp @@ -6,9 +6,7 @@ #include <vespa/messagebus/testlib/simplereply.h> #include <vespa/messagebus/testlib/slobrok.h> #include <vespa/messagebus/testlib/testserver.h> -#include <vespa/messagebus/errorcode.h> #include <vespa/messagebus/ireplyhandler.h> -#include <vespa/messagebus/network/identity.h> #include <vespa/messagebus/routing/routingcontext.h> #include <vespa/vespalib/testkit/testapp.h> #include <vespa/vespalib/component/vtag.h> @@ -29,12 +27,15 @@ Test::Main() { // test protocol IRoutingPolicy::UP bogus = protocol.createPolicy("bogus", ""); - EXPECT_TRUE(bogus.get() == 0); + EXPECT_FALSE(bogus); } TEST_FLUSH(); { // test SimpleMessage - Message::UP msg(new SimpleMessage("test")); + EXPECT_EQUAL(56u, sizeof(Routable)); + EXPECT_EQUAL(104u, sizeof(Message)); + EXPECT_EQUAL(184u, sizeof(SimpleMessage)); + auto msg = std::make_unique<SimpleMessage>("test"); EXPECT_TRUE(!msg->isReply()); EXPECT_TRUE(msg->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(msg->getType() == SimpleProtocol::MESSAGE); @@ -42,7 +43,7 @@ Test::Main() Blob b = protocol.encode(version, *msg); EXPECT_TRUE(b.size() > 0); Routable::UP tmp = protocol.decode(version, BlobRef(b)); - ASSERT_TRUE(tmp.get() != 0); + ASSERT_TRUE(tmp); EXPECT_TRUE(!tmp->isReply()); EXPECT_TRUE(tmp->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(tmp->getType() == SimpleProtocol::MESSAGE); @@ -51,7 +52,10 @@ Test::Main() TEST_FLUSH(); { // test SimpleReply - Reply::UP reply(new SimpleReply("reply")); + EXPECT_EQUAL(56u, sizeof(Routable)); + EXPECT_EQUAL(96u, sizeof(Reply)); + EXPECT_EQUAL(160u, sizeof(SimpleReply)); + auto reply = std::make_unique<SimpleReply>("reply"); EXPECT_TRUE(reply->isReply()); EXPECT_TRUE(reply->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(reply->getType() == SimpleProtocol::REPLY); @@ -59,7 +63,7 @@ Test::Main() Blob b = protocol.encode(version, *reply); EXPECT_TRUE(b.size() > 0); Routable::UP tmp = protocol.decode(version, BlobRef(b)); - ASSERT_TRUE(tmp.get() != 0); + ASSERT_TRUE(tmp); EXPECT_TRUE(tmp->isReply()); EXPECT_TRUE(tmp->getProtocol() == SimpleProtocol::NAME); EXPECT_TRUE(tmp->getType() == SimpleProtocol::REPLY); diff --git a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp index 344d38c8263..f97fc6a0010 100644 --- a/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp +++ b/messagebus/src/tests/trace-roundtrip/trace-roundtrip.cpp @@ -120,6 +120,6 @@ Test::Main() .addChild("Server reply") .addChild("Proxy reply") .addChild("Client reply"); - EXPECT_TRUE(reply->getTrace().getRoot().encode() == t.encode()); + EXPECT_TRUE(reply->getTrace().encode() == t.encode()); TEST_DONE(); } diff --git a/messagebus/src/vespa/messagebus/network/rpcsend.cpp b/messagebus/src/vespa/messagebus/network/rpcsend.cpp index dca7f0c997f..86c9b139f1a 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsend.cpp +++ b/messagebus/src/vespa/messagebus/network/rpcsend.cpp @@ -178,7 +178,7 @@ RPCSend::doRequestDone(FRT_RPCRequest *req) { } } else { FRT_Values &ret = *req->GetReturn(); - reply = createReply(ret, serviceName, error, trace.getRoot()); + reply = createReply(ret, serviceName, error, trace); } if (trace.shouldTrace(TraceLevel::SEND_RECEIVE)) { trace.trace(TraceLevel::SEND_RECEIVE, diff --git a/messagebus/src/vespa/messagebus/network/rpcsend.h b/messagebus/src/vespa/messagebus/network/rpcsend.h index f3a9177d236..1ccdea6fbc5 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsend.h +++ b/messagebus/src/vespa/messagebus/network/rpcsend.h @@ -12,7 +12,7 @@ class FRT_ReflectionBuilder; namespace vespalib::slime { struct Cursor; } namespace vespalib { struct Memory; } -namespace vespalib { class TraceNode; } +namespace vespalib { class Trace; } namespace mbus { class Error; @@ -56,7 +56,7 @@ protected: virtual void build(FRT_ReflectionBuilder & builder) = 0; virtual std::unique_ptr<Reply> createReply(const FRT_Values & response, const string & serviceName, - Error & error, vespalib::TraceNode & rootTrace) const = 0; + Error & error, vespalib::Trace & trace) const = 0; virtual void encodeRequest(FRT_RPCRequest &req, const vespalib::Version &version, const Route & route, const RPCServiceAddress & address, const Message & msg, uint32_t traceLevel, const PayLoadFiller &filler, duration timeRemaining) const = 0; diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp b/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp index 388ab3309c4..a8f7b47f3e3 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp +++ b/messagebus/src/vespa/messagebus/network/rpcsendv1.cpp @@ -115,7 +115,7 @@ RPCSendV1::toParams(const FRT_Values &args) const std::unique_ptr<Reply> -RPCSendV1::createReply(const FRT_Values & ret, const string & serviceName, Error & error, vespalib::TraceNode & rootTrace) const +RPCSendV1::createReply(const FRT_Values & ret, const string & serviceName, Error & error, vespalib::Trace & trace) const { vespalib::Version version = vespalib::Version(ret[0]._string._str); double retryDelay = ret[1]._double; @@ -127,7 +127,7 @@ RPCSendV1::createReply(const FRT_Values & ret, const string & serviceName, Error uint32_t errorServicesLen = ret[4]._string_array._len; const char *protocolName = ret[5]._string._str; BlobRef payload(ret[6]._data._buf, ret[6]._data._len); - const char *trace = ret[7]._string._str; + const char *traceStr = ret[7]._string._str; Reply::UP reply; if (payload.size() > 0) { @@ -141,7 +141,7 @@ RPCSendV1::createReply(const FRT_Values & ret, const string & serviceName, Error reply->addError(Error(errorCodes[i], errorMessages[i]._str, errorServices[i]._len > 0 ? errorServices[i]._str : serviceName.c_str())); } - rootTrace.addChild(TraceNode::decode(trace)); + trace.addChild(TraceNode::decode(traceStr)); return reply; } @@ -163,7 +163,7 @@ RPCSendV1::createResponse(FRT_Values & ret, const string & version, Reply & repl ret.AddString(reply.getProtocol().c_str()); ret.AddData(std::move(payload.payload()), payload.size()); if (reply.getTrace().getLevel() > 0) { - ret.AddString(reply.getTrace().getRoot().encode().c_str()); + ret.AddString(reply.getTrace().encode().c_str()); } else { ret.AddString(""); } diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv1.h b/messagebus/src/vespa/messagebus/network/rpcsendv1.h index 249acc50e0c..f9b4b0bdfe8 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsendv1.h +++ b/messagebus/src/vespa/messagebus/network/rpcsendv1.h @@ -17,7 +17,7 @@ private: const PayLoadFiller &filler, duration timeRemaining) const override; std::unique_ptr<Reply> createReply(const FRT_Values & response, const string & serviceName, - Error & error, vespalib::TraceNode & rootTrace) const override; + Error & error, vespalib::Trace & trace) const override; void createResponse(FRT_Values & ret, const string & version, Reply & reply, Blob payload) const override; }; diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp b/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp index f7303ece20f..16175fea1c2 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp +++ b/messagebus/src/vespa/messagebus/network/rpcsendv2.cpp @@ -188,7 +188,7 @@ RPCSendV2::toParams(const FRT_Values &args) const std::unique_ptr<Reply> RPCSendV2::createReply(const FRT_Values & ret, const string & serviceName, - Error & error, vespalib::TraceNode & rootTrace) const + Error & error, vespalib::Trace & rootTrace) const { uint8_t encoding = ret[3]._intval8; uint32_t uncompressedSize = ret[4]._intval32; @@ -240,7 +240,7 @@ RPCSendV2::createResponse(FRT_Values & ret, const string & version, Reply & repl root.setString(PROTOCOL_F, reply.getProtocol()); root.setData(BLOB_F, vespalib::Memory(payload.data(), payload.size())); if (reply.getTrace().getLevel() > 0) { - root.setString(TRACE_F, reply.getTrace().getRoot().encode()); + root.setString(TRACE_F, reply.getTrace().encode()); } if (reply.getNumErrors() > 0) { diff --git a/messagebus/src/vespa/messagebus/network/rpcsendv2.h b/messagebus/src/vespa/messagebus/network/rpcsendv2.h index c48aa90a9fb..da4154e70a8 100644 --- a/messagebus/src/vespa/messagebus/network/rpcsendv2.h +++ b/messagebus/src/vespa/messagebus/network/rpcsendv2.h @@ -17,7 +17,7 @@ private: const PayLoadFiller &filler, duration timeRemaining) const override; std::unique_ptr<Reply> createReply(const FRT_Values & response, const string & serviceName, - Error & error, vespalib::TraceNode & rootTrace) const override; + Error & error, vespalib::Trace & trace) const override; void createResponse(FRT_Values & ret, const string & version, Reply & reply, Blob payload) const override; }; diff --git a/messagebus/src/vespa/messagebus/routable.h b/messagebus/src/vespa/messagebus/routable.h index 50cb4e090ce..2c83daded1e 100644 --- a/messagebus/src/vespa/messagebus/routable.h +++ b/messagebus/src/vespa/messagebus/routable.h @@ -24,9 +24,9 @@ namespace mbus { */ class Routable { private: - Context _context; - CallStack _stack; - Trace _trace; + Context _context; + CallStack _stack; + Trace _trace; public: /** @@ -90,6 +90,7 @@ public: * @return Trace object */ Trace &getTrace() { return _trace; } + Trace && steal_trace() { return std::move(_trace); } /** * Access the Trace object for this Routable. The Trace is part of the @@ -106,7 +107,7 @@ public: * * @param trace The trace to set. */ - void setTrace(const Trace &trace) { _trace = trace; } + void setTrace(Trace &&trace) { _trace = std::move(trace); } /** * Swaps the state that makes this routable unique to another routable. The diff --git a/messagebus/src/vespa/messagebus/routing/routingnode.cpp b/messagebus/src/vespa/messagebus/routing/routingnode.cpp index f24afbc07ca..5a70f510dcc 100644 --- a/messagebus/src/vespa/messagebus/routing/routingnode.cpp +++ b/messagebus/src/vespa/messagebus/routing/routingnode.cpp @@ -184,10 +184,7 @@ RoutingNode::setReply(Reply::UP reply) { if (reply) { _shouldRetry = _resender != nullptr && _resender->shouldRetry(*reply); - if ( ! reply->getTrace().getRoot().isEmpty()) { - _trace.getRoot().addChild(std::move(reply->getTrace().getRoot())); - reply->getTrace().clear(); - } + _trace.addChild(reply->steal_trace()); } _reply = std::move(reply); } @@ -268,14 +265,12 @@ RoutingNode::notifyMerge() // Merges the trace information from all children into this. This method takes care not to spend cycles // manipulating the trace in case tracing is disabled. if (_trace.getLevel() > 0) { - TraceNode tail; + Trace tail; for (auto * child : _children) { - TraceNode &root = child->_trace.getRoot(); - tail.addChild(root); - root.clear(); + tail.addChild(std::move(child->_trace)); } tail.setStrict(false); - _trace.getRoot().addChild(tail); + _trace.addChild(std::move(tail)); } // Execute the {@link RoutingPolicy#merge(RoutingContext)} method of the current routing policy. If a diff --git a/messagebus/src/vespa/messagebus/sendproxy.cpp b/messagebus/src/vespa/messagebus/sendproxy.cpp index c884932664b..ff514f788cd 100644 --- a/messagebus/src/vespa/messagebus/sendproxy.cpp +++ b/messagebus/src/vespa/messagebus/sendproxy.cpp @@ -53,11 +53,10 @@ SendProxy::handleReply(Reply::UP reply) } else if (logger.wants(ns_log::Logger::spam)) { LOG(spam, "Trace for reply:\n%s", reply->getTrace().toString().c_str()); } - Trace empty; - trace.swap(empty); + trace.clear(); } else if (trace.getLevel() > 0) { - trace.getRoot().addChild(reply->getTrace().getRoot()); - trace.getRoot().normalize(); + trace.addChild(reply->steal_trace()); + trace.normalize(); } reply->swapState(*_msg); reply->setMessage(std::move(_msg)); diff --git a/messagebus_test/src/tests/trace/trace.cpp b/messagebus_test/src/tests/trace/trace.cpp index 334f00745da..1ab30303d2c 100644 --- a/messagebus_test/src/tests/trace/trace.cpp +++ b/messagebus_test/src/tests/trace/trace.cpp @@ -100,14 +100,14 @@ Test::Main() Reply::UP reply; SourceSession::UP ss = mb.getMessageBus().createSourceSession(src, SourceSessionParams()); for (int i = 0; i < 50; ++i) { - Message::UP msg(new SimpleMessage("test")); + auto msg = std::make_unique<SimpleMessage>("test"); msg->getTrace().setLevel(1); ss->send(std::move(msg), "test"); reply = src.getReply(10s); if (reply) { - reply->getTrace().getRoot().normalize(); + reply->getTrace().normalize(); // resending breaks the trace, so retry until it has expected form - if (!reply->hasErrors() && reply->getTrace().getRoot().encode() == expect.encode()) { + if (!reply->hasErrors() && reply->getTrace().encode() == expect.encode()) { break; } } @@ -116,7 +116,7 @@ Test::Main() } EXPECT_TRUE(!reply->hasErrors()); - EXPECT_EQUAL(reply->getTrace().getRoot().encode(), expect.encode()); + EXPECT_EQUAL(reply->getTrace().encode(), expect.encode()); EXPECT_TRUE(system((ctl_script + " stop all").c_str()) == 0); TEST_DONE(); } diff --git a/persistence/src/vespa/persistence/spi/context.h b/persistence/src/vespa/persistence/spi/context.h index 8c31439ee75..a1cb9e68f31 100644 --- a/persistence/src/vespa/persistence/spi/context.h +++ b/persistence/src/vespa/persistence/spi/context.h @@ -76,6 +76,7 @@ public: return _readConsistency; } + vespalib::Trace && steal_trace() { return std::move(_trace); } vespalib::Trace& getTrace() { return _trace; } const vespalib::Trace& getTrace() const { return _trace; } diff --git a/storage/src/tests/visiting/memory_bounded_trace_test.cpp b/storage/src/tests/visiting/memory_bounded_trace_test.cpp index 543531ef55a..e43fa702fb5 100644 --- a/storage/src/tests/visiting/memory_bounded_trace_test.cpp +++ b/storage/src/tests/visiting/memory_bounded_trace_test.cpp @@ -32,12 +32,12 @@ TEST(MemoryBoundedTraceTest, memory_used_is_accumulated_recursively_for_non_leaf TEST(MemoryBoundedTraceTest, trace_nodes_can_be_moved_and_implicitly_cleared) { MemoryBoundedTrace trace(100); EXPECT_TRUE(trace.add(mbus::TraceNode("hello world", epoch))); - mbus::TraceNode target; + mbus::Trace target; trace.moveTraceTo(target); EXPECT_EQ(1, target.getNumChildren()); EXPECT_EQ(0, trace.getApproxMemoryUsed()); - mbus::TraceNode emptinessCheck; + mbus::Trace emptinessCheck; trace.moveTraceTo(emptinessCheck); EXPECT_EQ(0, emptinessCheck.getNumChildren()); } @@ -52,7 +52,7 @@ TEST(MemoryBoundedTraceTest, trace_nodes_can_be_moved_and_implicitly_cleared) { TEST(MemoryBoundedTraceTest, moved_trace_tree_is_marked_as_strict) { MemoryBoundedTrace trace(100); EXPECT_TRUE(trace.add(mbus::TraceNode("hello world", epoch))); - mbus::TraceNode target; + mbus::Trace target; trace.moveTraceTo(target); EXPECT_EQ(1, target.getNumChildren()); EXPECT_TRUE(target.getChild(0).isStrict()); @@ -69,7 +69,7 @@ TEST(MemoryBoundedTraceTest, can_not_add_more_nodes_when_memory_used_exceeds_upp "the freeway", epoch))); EXPECT_EQ(11, trace.getApproxMemoryUsed()); - mbus::TraceNode target; + mbus::Trace target; trace.moveTraceTo(target); // Twice nested node (root -> added trace tree -> leaf with txt). EXPECT_EQ(1, target.getNumChildren()); @@ -82,7 +82,7 @@ TEST(MemoryBoundedTraceTest, moved_tree_includes_stats_node_when_nodes_omitted) EXPECT_TRUE(trace.add(mbus::TraceNode("abcdef", epoch))); EXPECT_FALSE(trace.add(mbus::TraceNode("ghijkjlmn", epoch))); - mbus::TraceNode target; + mbus::Trace target; trace.moveTraceTo(target); EXPECT_EQ(1, target.getNumChildren()); EXPECT_EQ(2, target.getChild(0).getNumChildren()); diff --git a/storage/src/tests/visiting/visitortest.cpp b/storage/src/tests/visiting/visitortest.cpp index 72668564345..f727cdf8eb2 100644 --- a/storage/src/tests/visiting/visitortest.cpp +++ b/storage/src/tests/visiting/visitortest.cpp @@ -799,15 +799,16 @@ TEST_F(VisitorTest, no_mbus_tracing_if_trace_level_is_zero) { cmd->getTrace().setLevel(0); std::shared_ptr<api::CreateVisitorReply> reply; ASSERT_NO_FATAL_FAILURE(doCompleteVisitingSession(cmd, reply)); - EXPECT_TRUE(reply->getTrace().getRoot().isEmpty()); + EXPECT_TRUE(reply->getTrace().isEmpty()); } TEST_F(VisitorTest, reply_contains_trace_if_trace_level_above_zero) { std::shared_ptr<api::CreateVisitorCommand> cmd(makeCreateVisitor()); cmd->getTrace().setLevel(1); + cmd->getTrace().trace(1,"at least one trace."); std::shared_ptr<api::CreateVisitorReply> reply; ASSERT_NO_FATAL_FAILURE(doCompleteVisitingSession(cmd, reply)); - EXPECT_FALSE(reply->getTrace().getRoot().isEmpty()); + EXPECT_FALSE(reply->getTrace().isEmpty()); } TEST_F(VisitorTest, no_more_iterators_sent_while_memory_used_above_limit) { diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp index c4fb9b8228c..c72e5731a59 100644 --- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp @@ -144,9 +144,7 @@ GetOperation::onReceive(DistributorMessageSender& sender, const std::shared_ptr< LOG(debug, "Received %s", msg->toString(true).c_str()); - if ( ! getreply->getTrace().getRoot().isEmpty()) { - _msg->getTrace().getRoot().addChild(getreply->getTrace().getRoot()); - } + _msg->getTrace().addChild(getreply->steal_trace()); bool allDone = true; for (auto& response : _responses) { for (uint32_t i = 0; i < response.second.size(); i++) { diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp index 3866ee4e6f7..d7dd0be7f4f 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.cpp @@ -27,21 +27,22 @@ TwoPhaseUpdateOperation::TwoPhaseUpdateOperation( DistributorMetricSet& metrics, SequencingHandle sequencingHandle) : SequencedOperation(std::move(sequencingHandle)), - _updateMetric(metrics.updates[msg->getLoadType()]), - _putMetric(metrics.update_puts[msg->getLoadType()]), - _getMetric(metrics.update_gets[msg->getLoadType()]), - _metadata_get_metrics(metrics.update_metadata_gets[msg->getLoadType()]), - _updateCmd(std::move(msg)), - _updateReply(), - _manager(manager), - _bucketSpace(bucketSpace), - _sendState(SendState::NONE_SENT), - _mode(Mode::FAST_PATH), - _single_get_latency_timer(), - _fast_path_repair_source_node(0xffff), - _use_initial_cheap_metadata_fetch_phase( + _updateMetric(metrics.updates[msg->getLoadType()]), + _putMetric(metrics.update_puts[msg->getLoadType()]), + _getMetric(metrics.update_gets[msg->getLoadType()]), + _metadata_get_metrics(metrics.update_metadata_gets[msg->getLoadType()]), + _updateCmd(std::move(msg)), + _updateReply(), + _manager(manager), + _bucketSpace(bucketSpace), + _sendState(SendState::NONE_SENT), + _mode(Mode::FAST_PATH), + _trace(_updateCmd->getTrace().getLevel()), + _single_get_latency_timer(), + _fast_path_repair_source_node(0xffff), + _use_initial_cheap_metadata_fetch_phase( _manager.getDistributor().getConfig().enable_metadata_only_fetch_phase_for_inconsistent_updates()), - _replySent(false) + _replySent(false) { document::BucketIdFactory idFactory; _updateDocBucketId = idFactory.getBucketId(_updateCmd->getDocumentId()); @@ -132,9 +133,7 @@ TwoPhaseUpdateOperation::sendReply( std::shared_ptr<api::StorageReply>& reply) { assert(!_replySent); - if (!_trace.isEmpty()) { - reply->getTrace().getRoot().addChild(_trace); - } + reply->getTrace().addChild(std::move(_trace)); sender.sendReply(reply); _replySent = true; } @@ -645,11 +644,9 @@ TwoPhaseUpdateOperation::satisfiesUpdateTimestampConstraint(api::Timestamp ts) c } void -TwoPhaseUpdateOperation::addTraceFromReply(const api::StorageReply& reply) +TwoPhaseUpdateOperation::addTraceFromReply(api::StorageReply & reply) { - if ( ! reply.getTrace().getRoot().isEmpty()) { - _trace.addChild(reply.getTrace().getRoot()); - } + _trace.addChild(reply.steal_trace()); } void diff --git a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h index 2d8f3e8494d..8527ff0ffba 100644 --- a/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/twophaseupdateoperation.h @@ -125,7 +125,7 @@ private: DistributorMessageSender& sender, const document::Document& candidateDoc); bool satisfiesUpdateTimestampConstraint(api::Timestamp) const; - void addTraceFromReply(const api::StorageReply& reply); + void addTraceFromReply(api::StorageReply& reply); bool hasTasCondition() const noexcept; void replyWithTasFailure(DistributorMessageSender& sender, vespalib::stringref message); @@ -146,7 +146,7 @@ private: SentMessageMap _sentMessageMap; SendState _sendState; Mode _mode; - mbus::TraceNode _trace; + mbus::Trace _trace; document::BucketId _updateDocBucketId; std::vector<std::pair<document::BucketId, uint16_t>> _replicas_at_get_send_time; std::optional<framework::MilliSecTimer> _single_get_latency_timer; diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp index 5a03e05d563..616fd312723 100644 --- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.cpp @@ -171,7 +171,7 @@ VisitorOperation::onReceive( { api::CreateVisitorReply& reply = static_cast<api::CreateVisitorReply&>(*r); - _trace.add(reply.getTrace().getRoot()); + _trace.add(reply.steal_trace()); SentMessagesMap::iterator iter = _sentMessages.find(reply.getMsgId()); assert(iter != _sentMessages.end()); @@ -828,8 +828,8 @@ VisitorOperation::sendReply(const api::ReturnCode& code, DistributorMessageSende { if (!_sentReply) { // Send create visitor reply - api::CreateVisitorReply::SP reply(new api::CreateVisitorReply(*_msg)); - _trace.moveTraceTo(reply->getTrace().getRoot()); + auto reply = std::make_shared<api::CreateVisitorReply>(*_msg); + _trace.moveTraceTo(reply->getTrace()); reply->setLastBucket(getLastBucketVisited()); reply->setResult(code); diff --git a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h index 42b0bd56b9e..24c7157b481 100644 --- a/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/visitoroperation.h @@ -38,7 +38,7 @@ public: const Config& config, VisitorMetricSet& metrics); - ~VisitorOperation(); + ~VisitorOperation() override; void onClose(DistributorMessageSender& sender) override; void onStart(DistributorMessageSender& sender) override; diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp index 6362db6c002..584cb379400 100644 --- a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp +++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp @@ -22,6 +22,7 @@ PersistenceMessageTrackerImpl::PersistenceMessageTrackerImpl( _reply(std::move(reply)), _manager(link), _revertTimestamp(revertTimestamp), + _trace(_reply->getTrace().getLevel()), _requestTimer(link.getClock()), _n_persistence_replies_total(0), _n_successful_persistence_replies(0), @@ -163,25 +164,18 @@ PersistenceMessageTrackerImpl::addBucketInfoFromReply( bucket.toString().c_str(), bucketInfo.toString().c_str(), node); - _remapBucketInfo[bucket].push_back( - BucketCopy(_manager.getUniqueTimestamp(), - node, - bucketInfo)); + _remapBucketInfo[bucket].emplace_back(_manager.getUniqueTimestamp(), node, bucketInfo); } else { LOG(debug, "Bucket %s: Received bucket info %s from node %d", bucket.toString().c_str(), bucketInfo.toString().c_str(), node); - _bucketInfo[bucket].push_back( - BucketCopy(_manager.getUniqueTimestamp(), - node, - bucketInfo)); + _bucketInfo[bucket].emplace_back(_manager.getUniqueTimestamp(), node, bucketInfo); } } void -PersistenceMessageTrackerImpl::logSuccessfulReply(uint16_t node, - const api::BucketInfoReply& reply) const +PersistenceMessageTrackerImpl::logSuccessfulReply(uint16_t node, const api::BucketInfoReply& reply) const { LOG(spam, "Bucket %s: Received successful reply %s", reply.getBucketId().toString().c_str(), @@ -230,7 +224,7 @@ PersistenceMessageTrackerImpl::sendReply(MessageSender& sender) updateMetrics(); _trace.setStrict(false); if ( ! _trace.isEmpty()) { - _reply->getTrace().getRoot().addChild(_trace); + _reply->getTrace().addChild(std::move(_trace)); } sender.sendReply(_reply); @@ -292,9 +286,7 @@ PersistenceMessageTrackerImpl::updateFromReply( api::BucketInfoReply& reply, uint16_t node) { - if ( ! reply.getTrace().getRoot().isEmpty()) { - _trace.addChild(reply.getTrace().getRoot()); - } + _trace.addChild(reply.steal_trace()); if (reply.getType() == api::MessageType::CREATEBUCKET_REPLY) { handleCreateBucketReply(reply, node); diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.h b/storage/src/vespa/storage/distributor/persistencemessagetracker.h index 4392aa1fc30..cf9f4017eda 100644 --- a/storage/src/vespa/storage/distributor/persistencemessagetracker.h +++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.h @@ -73,7 +73,7 @@ private: DistributorComponent& _manager; api::Timestamp _revertTimestamp; std::vector<BucketNodePair> _revertNodes; - mbus::TraceNode _trace; + mbus::Trace _trace; framework::MilliSecTimer _requestTimer; uint32_t _n_persistence_replies_total; uint32_t _n_successful_persistence_replies; diff --git a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp index cba3969cd68..54c1c7606b2 100644 --- a/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp +++ b/storage/src/vespa/storage/persistence/filestorage/filestormanager.cpp @@ -676,7 +676,7 @@ FileStorManager::onInternal(const shared_ptr<api::InternalCommand>& msg) spi::Context context(msg->getLoadType(), msg->getPriority(), msg->getTrace().getLevel()); shared_ptr<DestroyIteratorCommand> cmd(std::static_pointer_cast<DestroyIteratorCommand>(msg)); _provider->destroyIterator(cmd->getIteratorId(), context); - msg->getTrace().getRoot().addChild(context.getTrace().getRoot()); + msg->getTrace().addChild(context.steal_trace()); return true; } case ReadBucketList::ID: diff --git a/storage/src/vespa/storage/persistence/persistenceutil.cpp b/storage/src/vespa/storage/persistence/persistenceutil.cpp index 42d67573763..9a7107fb583 100644 --- a/storage/src/vespa/storage/persistence/persistenceutil.cpp +++ b/storage/src/vespa/storage/persistence/persistenceutil.cpp @@ -93,9 +93,7 @@ MessageTracker::sendReply() { _msg->toString(true).c_str(), vespalib::to_s(duration)); } if (hasReply()) { - if ( ! _context.getTrace().getRoot().isEmpty()) { - getReply().getTrace().getRoot().addChild(_context.getTrace().getRoot()); - } + getReply().getTrace().addChild(_context.steal_trace()); if (_updateBucketInfo) { if (getReply().getResult().success()) { _env.setBucketInfo(*this, _bucketLock->getBucket()); @@ -104,13 +102,10 @@ MessageTracker::sendReply() { if (getReply().getResult().success()) { _metric->latency.addValue(_timer.getElapsedTimeAsDouble()); } - LOG(spam, "Sending reply up: %s %" PRIu64, - getReply().toString().c_str(), getReply().getMsgId()); + LOG(spam, "Sending reply up: %s %" PRIu64, getReply().toString().c_str(), getReply().getMsgId()); _replySender.sendReplyDirectly(std::move(_reply)); } else { - if ( ! _context.getTrace().getRoot().isEmpty()) { - _msg->getTrace().getRoot().addChild(_context.getTrace().getRoot()); - } + _msg->getTrace().addChild(_context.steal_trace()); } } diff --git a/storage/src/vespa/storage/storageserver/communicationmanager.cpp b/storage/src/vespa/storage/storageserver/communicationmanager.cpp index a35b9d1d59a..940bffdbcc6 100644 --- a/storage/src/vespa/storage/storageserver/communicationmanager.cpp +++ b/storage/src/vespa/storage/storageserver/communicationmanager.cpp @@ -102,7 +102,7 @@ CommunicationManager::handleMessage(std::unique_ptr<mbus::Message> msg) return; } - cmd->setTrace(docMsgPtr->getTrace()); + cmd->setTrace(docMsgPtr->steal_trace()); cmd->setTransportContext(std::make_unique<StorageTransportContext>(std::move(docMsgPtr))); enqueue_or_process(std::move(cmd)); @@ -114,7 +114,7 @@ CommunicationManager::handleMessage(std::unique_ptr<mbus::Message> msg) //TODO: Can it be moved ? std::shared_ptr<api::StorageCommand> cmd = storMsgPtr->getCommand(); cmd->setTimeout(storMsgPtr->getTimeRemaining()); - cmd->setTrace(storMsgPtr->getTrace()); + cmd->setTrace(storMsgPtr->steal_trace()); cmd->setTransportContext(std::make_unique<StorageTransportContext>(std::move(storMsgPtr))); enqueue_or_process(std::move(cmd)); @@ -203,12 +203,12 @@ CommunicationManager::handleReply(std::unique_ptr<mbus::Reply> reply) _docApiConverter.toStorageAPI(static_cast<documentapi::DocumentReply&>(*reply), *originalCommand)); if (sar) { - sar->setTrace(reply->getTrace()); + sar->setTrace(reply->steal_trace()); receiveStorageReply(sar); } } else if (protocolName == mbusprot::StorageProtocol::NAME) { mbusprot::StorageReply* sr(static_cast<mbusprot::StorageReply*>(reply.get())); - sr->getReply()->setTrace(reply->getTrace()); + sr->getReply()->setTrace(reply->steal_trace()); receiveStorageReply(sr->getReply()); } else { LOGBM(warning, "Received unsupported reply type %d for protocol '%s'.", @@ -583,7 +583,7 @@ CommunicationManager::sendCommand( cmd->setContext(mbus::Context(msg->getMsgId())); cmd->setRetryEnabled(false); cmd->setTimeRemaining(msg->getTimeout()); - cmd->setTrace(msg->getTrace()); + cmd->setTrace(msg->steal_trace()); sendMessageBusMessage(msg, std::move(cmd), address.getRoute()); } break; @@ -596,7 +596,7 @@ CommunicationManager::sendCommand( if (mbusMsg) { MBUS_TRACE(msg->getTrace(), 7, "Communication manager: Converted OK"); - mbusMsg->setTrace(msg->getTrace()); + mbusMsg->setTrace(msg->steal_trace()); mbusMsg->setRetryEnabled(false); { @@ -690,13 +690,13 @@ CommunicationManager::sendMessageBusReply( if (reply->getResult().getResult() == api::ReturnCode::WRONG_DISTRIBUTION) { replyUP = std::make_unique<documentapi::WrongDistributionReply>(reply->getResult().getMessage()); replyUP->swapState(*context._docAPIMsg); - replyUP->setTrace(reply->getTrace()); + replyUP->setTrace(reply->steal_trace()); replyUP->addError(mbus::Error(documentapi::DocumentProtocol::ERROR_WRONG_DISTRIBUTION, reply->getResult().getMessage())); } else { replyUP = context._docAPIMsg->createReply(); replyUP->swapState(*context._docAPIMsg); - replyUP->setTrace(reply->getTrace()); + replyUP->setTrace(reply->steal_trace()); replyUP->setMessage(std::move(context._docAPIMsg)); _docApiConverter.transferReplyState(*reply, *replyUP); } @@ -707,7 +707,7 @@ CommunicationManager::sendMessageBusReply( } replyUP->swapState(*context._storageProtocolMsg); - replyUP->setTrace(reply->getTrace()); + replyUP->setTrace(reply->steal_trace()); replyUP->setMessage(std::move(context._storageProtocolMsg)); } diff --git a/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp b/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp index cb1d0380bf1..c465315a5a6 100644 --- a/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp +++ b/storage/src/vespa/storage/storageserver/rpc/storage_api_rpc_service.cpp @@ -208,7 +208,7 @@ void StorageApiRpcService::encode_rpc_v1_response(FRT_RPCRequest& request, api:: // TODO skip encoding header altogether if no relevant fields set? protobuf::ResponseHeader hdr; if (reply.getTrace().getLevel() > 0) { - hdr.set_trace_payload(reply.getTrace().getRoot().encode()); + hdr.set_trace_payload(reply.getTrace().encode()); } // TODO consistent naming... encode_header_into_rpc_params(hdr, *ret); @@ -289,7 +289,7 @@ void StorageApiRpcService::RequestDone(FRT_RPCRequest* raw_req) { assert(reply); if (!hdr.trace_payload().empty()) { - cmd.getTrace().getRoot().addChild(mbus::TraceNode::decode(hdr.trace_payload())); + cmd.getTrace().addChild(mbus::TraceNode::decode(hdr.trace_payload())); } if (cmd.getTrace().shouldTrace(TraceLevel::SEND_RECEIVE)) { cmd.getTrace().trace(TraceLevel::SEND_RECEIVE, diff --git a/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp b/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp index 9f9eb2942c1..05bb026f238 100644 --- a/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp +++ b/storage/src/vespa/storage/visiting/memory_bounded_trace.cpp @@ -6,7 +6,7 @@ namespace storage { MemoryBoundedTrace::MemoryBoundedTrace(size_t softMemoryUpperBound) - : _node(), + : _trace(), _currentMemoryUsed(0), _omittedNodes(0), _omittedBytes(0), @@ -14,53 +14,49 @@ MemoryBoundedTrace::MemoryBoundedTrace(size_t softMemoryUpperBound) { } -namespace { - -size_t -computeTraceTreeMemoryUsage(const mbus::TraceNode& node) +bool +MemoryBoundedTrace::add(const mbus::TraceNode& node) { - if (node.isLeaf()) { - return node.getNote().size(); - } - size_t childSum = 0; - const uint32_t childCount = node.getNumChildren(); - for (uint32_t i = 0; i < childCount; ++i) { - childSum += computeTraceTreeMemoryUsage(node.getChild(i)); + const size_t nodeFootprint = node.computeMemoryUsage(); + + if (_currentMemoryUsed >= _softMemoryUpperBound) { + ++_omittedNodes; + _omittedBytes += nodeFootprint; + return false; } - return childSum; + _trace.addChild(vespalib::TraceNode(node)); + _currentMemoryUsed += nodeFootprint; + return true; } -} // anon ns - bool -MemoryBoundedTrace::add(const mbus::TraceNode& node) +MemoryBoundedTrace::add(mbus::Trace && node) { - const size_t nodeFootprint = computeTraceTreeMemoryUsage(node); + const size_t nodeFootprint = node.computeMemoryUsage(); if (_currentMemoryUsed >= _softMemoryUpperBound) { ++_omittedNodes; _omittedBytes += nodeFootprint; return false; } - _node.addChild(node); + _trace.addChild(std::move(node)); _currentMemoryUsed += nodeFootprint; return true; } void -MemoryBoundedTrace::moveTraceTo(mbus::TraceNode& out) +MemoryBoundedTrace::moveTraceTo(mbus::Trace& out) { - if (_node.isEmpty()) { + if (_trace.isEmpty()) { return; } if (_omittedNodes > 0) { - _node.addChild(vespalib::make_string( + _trace.trace(0, vespalib::make_string( "Trace too large; omitted %zu subsequent trace trees " "containing a total of %zu bytes", - _omittedNodes, _omittedBytes)); + _omittedNodes, _omittedBytes), false); } - out.addChild(_node); // XXX rvalue support should be added to TraceNode. - _node.clear(); + out.addChild(std::move(_trace)); _currentMemoryUsed = 0; _omittedNodes = 0; _omittedBytes = 0; diff --git a/storage/src/vespa/storage/visiting/memory_bounded_trace.h b/storage/src/vespa/storage/visiting/memory_bounded_trace.h index 71ce1be51ac..2c75c809af6 100644 --- a/storage/src/vespa/storage/visiting/memory_bounded_trace.h +++ b/storage/src/vespa/storage/visiting/memory_bounded_trace.h @@ -23,6 +23,7 @@ public: * otherwise. */ bool add(const mbus::TraceNode& node); + bool add(mbus::Trace && trace); /** * Append current trace tree to the output trace node and clear internal @@ -33,14 +34,14 @@ public: * * If current trace is empty, no nodes are added to `out`. */ - void moveTraceTo(mbus::TraceNode& out); + void moveTraceTo(mbus::Trace& out); size_t getApproxMemoryUsed() const noexcept { return _currentMemoryUsed; } private: - mbus::TraceNode _node; + mbus::Trace _trace; size_t _currentMemoryUsed; size_t _omittedNodes; size_t _omittedBytes; diff --git a/storage/src/vespa/storage/visiting/visitor.cpp b/storage/src/vespa/storage/visiting/visitor.cpp index 982bcd78f6f..9dee798de84 100644 --- a/storage/src/vespa/storage/visiting/visitor.cpp +++ b/storage/src/vespa/storage/visiting/visitor.cpp @@ -146,8 +146,7 @@ Visitor::BucketIterationState::~BucketIterationState() { if (_iteratorId != 0) { // Making the assumption that this is effectively nothrow. - std::shared_ptr<DestroyIteratorCommand> cmd( - new DestroyIteratorCommand(_iteratorId)); + auto cmd = std::make_shared<DestroyIteratorCommand>(_iteratorId); cmd->setLoadType(_visitor._initiatingCmd->getLoadType()); cmd->getTrace().setLevel(_visitor._traceLevel); cmd->setPriority(0); @@ -178,7 +177,7 @@ Visitor::VisitorTarget::VisitorTarget() { } -Visitor::VisitorTarget::~VisitorTarget() {} +Visitor::VisitorTarget::~VisitorTarget() = default; Visitor::Visitor(StorageComponent& component) : _component(component), @@ -372,10 +371,9 @@ Visitor::sendReplyOnce() std::shared_ptr<api::StorageReply> reply(_initiatingCmd->makeReply()); _hitCounter->updateVisitorStatistics(_visitorStatistics); - static_cast<api::CreateVisitorReply*>(reply.get()) - ->setVisitorStatistics(_visitorStatistics); + static_cast<api::CreateVisitorReply*>(reply.get())->setVisitorStatistics(_visitorStatistics); if (shouldAddMbusTrace()) { - _trace.moveTraceTo(reply->getTrace().getRoot()); + _trace.moveTraceTo(reply->getTrace()); } reply->setResult(_result); LOG(debug, "Sending %s", reply->toString(true).c_str()); @@ -603,15 +601,14 @@ bool Visitor::addBoundedTrace(uint32_t level, const vespalib::string &message) { mbus::Trace tempTrace; tempTrace.trace(level, message); - return _trace.add(tempTrace.getRoot()); + return _trace.add(std::move(tempTrace)); } void -Visitor::handleDocumentApiReply(mbus::Reply::UP reply, - VisitorThreadMetrics& metrics) +Visitor::handleDocumentApiReply(mbus::Reply::UP reply, VisitorThreadMetrics& metrics) { if (shouldAddMbusTrace()) { - _trace.add(reply->getTrace().getRoot()); + _trace.add(reply->steal_trace()); } mbus::Message::UP message = reply->getMessage(); @@ -832,7 +829,7 @@ Visitor::onGetIterReply(const std::shared_ptr<GetIterReply>& reply, } if (shouldAddMbusTrace()) { - _trace.add(reply->getTrace().getRoot()); + _trace.add(reply->steal_trace()); } LOG(debug, "Continuing visitor %s.", _id.c_str()); diff --git a/storageapi/src/tests/messageapi/storage_message_address_test.cpp b/storageapi/src/tests/messageapi/storage_message_address_test.cpp index c340cba4b28..9db282c98eb 100644 --- a/storageapi/src/tests/messageapi/storage_message_address_test.cpp +++ b/storageapi/src/tests/messageapi/storage_message_address_test.cpp @@ -31,6 +31,11 @@ TEST(StorageMessageAddressTest, storage_hash_covers_all_expected_fields) { hash_of("foo", lib::NodeType::DISTRIBUTOR, 0)); EXPECT_NE(hash_of("foo", lib::NodeType::STORAGE, 0), hash_of("foo", lib::NodeType::STORAGE, 1)); + + EXPECT_EQ(112u, sizeof(StorageMessageAddress)); + EXPECT_EQ(144u, sizeof(StorageMessage)); + EXPECT_EQ(80u, sizeof(documentapi::LoadType)); + EXPECT_EQ(16u, sizeof(mbus::Trace)); } } // storage::api diff --git a/storageapi/src/vespa/storageapi/messageapi/bucketreply.cpp b/storageapi/src/vespa/storageapi/messageapi/bucketreply.cpp index 1385fc331ac..e5fb7764aeb 100644 --- a/storageapi/src/vespa/storageapi/messageapi/bucketreply.cpp +++ b/storageapi/src/vespa/storageapi/messageapi/bucketreply.cpp @@ -7,8 +7,7 @@ using document::Bucket; using document::BucketId; -namespace storage { -namespace api { +namespace storage::api { BucketReply::BucketReply(const BucketCommand& cmd, const ReturnCode& code) @@ -42,5 +41,4 @@ BucketReply::print(std::ostream& out, bool verbose, } } -} // api -} // storage +} diff --git a/storageapi/src/vespa/storageapi/messageapi/storagecommand.cpp b/storageapi/src/vespa/storageapi/messageapi/storagecommand.cpp index d9bbf34141a..397c869f4fb 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagecommand.cpp +++ b/storageapi/src/vespa/storageapi/messageapi/storagecommand.cpp @@ -11,7 +11,6 @@ StorageCommand::StorageCommand(const StorageCommand& other) _timeout(other._timeout), _sourceIndex(other._sourceIndex) { - setTrace(other.getTrace()); } StorageCommand::StorageCommand(const MessageType& type, Priority p) diff --git a/storageapi/src/vespa/storageapi/messageapi/storagecommand.h b/storageapi/src/vespa/storageapi/messageapi/storagecommand.h index c835168c5b7..bd55ee57a8f 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagecommand.h +++ b/storageapi/src/vespa/storageapi/messageapi/storagecommand.h @@ -30,7 +30,7 @@ protected: public: DECLARE_POINTER_TYPEDEFS(StorageCommand); - virtual ~StorageCommand(); + ~StorageCommand() override; bool sourceIndexSet() const { return (_sourceIndex != 0xffff); } void setSourceIndex(uint16_t sourceIndex) { _sourceIndex = sourceIndex; } diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp index 0f5546a5510..700669739ce 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp +++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.cpp @@ -141,10 +141,10 @@ MessageType::print(std::ostream& out, bool verbose, const std::string& indent) c StorageMessageAddress::StorageMessageAddress(const mbus::Route& route) : _route(route), - _protocol(DOCUMENT), + _cluster(), _precomputed_storage_hash(0), - _cluster(""), _type(nullptr), + _protocol(DOCUMENT), _index(0xFFFF) { } @@ -163,10 +163,10 @@ createAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t i // TODO we ideally want this removed. Currently just in place to support usage as map key when emplacement not available StorageMessageAddress::StorageMessageAddress() : _route(), - _protocol(Protocol::STORAGE), - _precomputed_storage_hash(0), _cluster(), + _precomputed_storage_hash(0), _type(nullptr), + _protocol(Protocol::STORAGE), _index(0) {} @@ -174,10 +174,10 @@ StorageMessageAddress::StorageMessageAddress() StorageMessageAddress::StorageMessageAddress(vespalib::stringref cluster, const lib::NodeType& type, uint16_t index, Protocol protocol) : _route(), - _protocol(protocol), - _precomputed_storage_hash(0), _cluster(cluster), + _precomputed_storage_hash(0), _type(&type), + _protocol(protocol), _index(index) { std::vector<mbus::IHopDirective::SP> directives; @@ -269,20 +269,22 @@ StorageMessage::generateMsgId() StorageMessage::StorageMessage(const MessageType& type, Id id) : _type(type), _msgId(id), - _priority(NORMAL), _address(), _loadType(documentapi::LoadType::DEFAULT), - _approxByteSize(50) + _trace(), + _approxByteSize(50), + _priority(NORMAL) { } StorageMessage::StorageMessage(const StorageMessage& other, Id id) : _type(other._type), _msgId(id), - _priority(other._priority), _address(), _loadType(other._loadType), - _approxByteSize(other._approxByteSize) + _trace(other.getTrace().getLevel()), + _approxByteSize(other._approxByteSize), + _priority(other._priority) { } diff --git a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h index aa23d3a0cae..58f167f5002 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagemessage.h +++ b/storageapi/src/vespa/storageapi/messageapi/storagemessage.h @@ -268,12 +268,12 @@ public: enum Protocol { STORAGE, DOCUMENT }; private: - mbus::Route _route; - Protocol _protocol; + mbus::Route _route; + vespalib::string _cluster; // Used for internal VDS addresses only size_t _precomputed_storage_hash; - vespalib::string _cluster; const lib::NodeType* _type; + Protocol _protocol; uint16_t _index; public: @@ -359,12 +359,12 @@ protected: static Id generateMsgId(); const MessageType& _type; - Id _msgId; - Priority _priority; + Id _msgId; std::unique_ptr<StorageMessageAddress> _address; - documentapi::LoadType _loadType; - mbus::Trace _trace; - uint32_t _approxByteSize; + documentapi::LoadType _loadType; + vespalib::Trace _trace; + uint32_t _approxByteSize; + Priority _priority; StorageMessage(const MessageType& code, Id id); StorageMessage(const StorageMessage&, Id id); @@ -373,7 +373,7 @@ protected: public: StorageMessage& operator=(const StorageMessage&) = delete; StorageMessage(const StorageMessage&) = delete; - virtual ~StorageMessage(); + ~StorageMessage() override; Id getMsgId() const { return _msgId; } @@ -394,7 +394,7 @@ public: const StorageMessageAddress* getAddress() const { return _address.get(); } void setAddress(const StorageMessageAddress& address) { - _address.reset(new StorageMessageAddress(address)); + _address = std::make_unique<StorageMessageAddress>(address); } /** @@ -435,13 +435,14 @@ public: const documentapi::LoadType& getLoadType() const { return _loadType; } void setLoadType(const documentapi::LoadType& type) { _loadType = type; } + mbus::Trace && steal_trace() { return std::move(_trace); } mbus::Trace& getTrace() { return _trace; } const mbus::Trace& getTrace() const { return _trace; } /** Sets the trace object for this message. */ - void setTrace(const mbus::Trace &trace) { _trace = trace; } + void setTrace(vespalib::Trace && trace) { _trace = std::move(trace); } /** * Cheap version of tostring(). diff --git a/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp b/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp index 81cdadb3623..2bb9fabd7d5 100644 --- a/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp +++ b/storageapi/src/vespa/storageapi/messageapi/storagereply.cpp @@ -14,7 +14,12 @@ StorageReply::StorageReply(const StorageCommand& cmd, ReturnCode code) if (cmd.getAddress()) { setAddress(*cmd.getAddress()); } - setTrace(cmd.getTrace()); + // TODD do we really need copy construction + if ( ! cmd.getTrace().isEmpty()) { + setTrace(vespalib::Trace(cmd.getTrace())); + } else { + getTrace().setLevel(cmd.getTrace().getLevel()); + } setTransportContext(cmd.getTransportContext()); } diff --git a/vespalib/src/tests/trace/trace.cpp b/vespalib/src/tests/trace/trace.cpp index 92bee3231b0..992317b0289 100644 --- a/vespalib/src/tests/trace/trace.cpp +++ b/vespalib/src/tests/trace/trace.cpp @@ -146,25 +146,25 @@ TEST("testTraceLevel") t.setLevel(4); EXPECT_EQUAL(4u, t.getLevel()); t.trace(9, "no"); - EXPECT_EQUAL(0u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(0u, t.getNumChildren()); t.trace(8, "no"); - EXPECT_EQUAL(0u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(0u, t.getNumChildren()); t.trace(7, "no"); - EXPECT_EQUAL(0u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(0u, t.getNumChildren()); t.trace(6, "no"); - EXPECT_EQUAL(0u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(0u, t.getNumChildren()); t.trace(5, "no"); - EXPECT_EQUAL(0u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(0u, t.getNumChildren()); t.trace(4, "yes"); - EXPECT_EQUAL(1u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(1u, t.getNumChildren()); t.trace(3, "yes"); - EXPECT_EQUAL(2u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(2u, t.getNumChildren()); t.trace(2, "yes"); - EXPECT_EQUAL(3u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(3u, t.getNumChildren()); t.trace(1, "yes"); - EXPECT_EQUAL(4u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(4u, t.getNumChildren()); t.trace(0, "yes"); - EXPECT_EQUAL(5u, t.getRoot().getNumChildren()); + EXPECT_EQUAL(5u, t.getNumChildren()); } TEST("testCompact") @@ -261,10 +261,10 @@ TEST("testTraceDump") b1.addChild(b2); } for (int i = 0; i < 10; ++i) { - big.getRoot().addChild(b1); + big.addChild(TraceNode(b1)); } string normal = big.toString(); - string full = big.getRoot().toString(); + string full = big.toString(100000); EXPECT_GREATER(normal.size(), 30000u); EXPECT_LESS(normal.size(), 32000u); EXPECT_GREATER(full.size(), 50000u); diff --git a/vespalib/src/vespa/vespalib/trace/trace.cpp b/vespalib/src/vespa/vespalib/trace/trace.cpp index 8ca2ef6561c..be370aebbd2 100644 --- a/vespalib/src/vespa/vespalib/trace/trace.cpp +++ b/vespalib/src/vespa/vespalib/trace/trace.cpp @@ -6,43 +6,13 @@ namespace vespalib { -Trace::Trace() : - _level(0), - _root() +Trace::Trace(const Trace &rhs) + : _root(), + _level(rhs._level) { - // empty -} - -Trace::Trace(uint32_t level) : - _level(level), - _root() -{ - // empty -} - -Trace::~Trace() = default; - -Trace & -Trace::clear() -{ - _level = 0; - _root.clear(); - return *this; -} - -Trace & -Trace::swap(Trace &other) -{ - std::swap(_level, other._level); - _root.swap(other._root); - return *this; -} - -Trace & -Trace::setLevel(uint32_t level) -{ - _level = std::min(level, 9u); - return *this; + if (!rhs.isEmpty()) { + _root = std::make_unique<TraceNode>(rhs.getRoot()); + } } bool @@ -53,12 +23,36 @@ Trace::trace(uint32_t level, const string ¬e, bool addTime) } if (addTime) { struct timeval tv; - gettimeofday(&tv, NULL); - _root.addChild(make_string("[%ld.%06ld] %s", tv.tv_sec, static_cast<long>(tv.tv_usec), note.c_str())); + gettimeofday(&tv, nullptr); + ensureRoot().addChild(make_string("[%ld.%06ld] %s", tv.tv_sec, static_cast<long>(tv.tv_usec), note.c_str())); } else { - _root.addChild(note); + ensureRoot().addChild(note); } return true; } +string +Trace::toString(size_t limit) const { + return _root ? _root->toString(limit) : ""; +} + +string +Trace::encode() const { + return isEmpty() ? "" : getRoot().encode(); +} + +void +Trace::clear() { + _level = 0; + _root.reset(); +} + +TraceNode & +Trace::ensureRoot() { + if (!_root) { + _root = std::make_unique<TraceNode>(); + } + return *_root; +} + } // namespace vespalib diff --git a/vespalib/src/vespa/vespalib/trace/trace.h b/vespalib/src/vespa/vespalib/trace/trace.h index d1ca5c5d7e7..2f70785d77d 100644 --- a/vespalib/src/vespa/vespalib/trace/trace.h +++ b/vespalib/src/vespa/vespalib/trace/trace.h @@ -18,31 +18,23 @@ namespace vespalib { * information will be traced. */ class Trace { -private: - uint32_t _level; - TraceNode _root; - public: /** * Create an empty Trace with level set to 0 (no tracing) */ - Trace(); - ~Trace(); - - /** - * Create an empty trace with given level. - * - * @param level Level to set. - */ - explicit Trace(uint32_t level); + Trace() : Trace(0) {} + explicit Trace(uint32_t level) : _root(), _level(level) { } + Trace & operator = (Trace &&) = default; + Trace(Trace &&) = default; + Trace(const Trace &); + Trace & operator = (const Trace &) = delete; + ~Trace() = default; /** * Remove all trace information and set the trace level to 0. - * - * @return This, to allow chaining. */ - Trace &clear(); + void clear(); /** * Swap the internals of this with another. @@ -50,21 +42,16 @@ public: * @param other The trace to swap internals with. * @return This, to allow chaining. */ - Trace &swap(Trace &other); + Trace &swap(Trace &other) { + std::swap(_level, other._level); + _root.swap(other._root); + return *this; + } - /** - * Set the trace level. 0 means no tracing, 9 means enable all tracing. - * - * @param level The level to set. - * @return This, to allow chaining. - */ - Trace &setLevel(uint32_t level); + void setLevel(uint32_t level) { + _level = std::min(level, 9u); + } - /** - * Returns the trace level. - * - * @return The trace level. - */ uint32_t getLevel() const { return _level; } /** @@ -91,19 +78,29 @@ public: */ bool trace(uint32_t level, const string ¬e, bool addTime = true); - /** - * Returns the root of the trace tree. - * - * @return The root. - */ - TraceNode &getRoot() { return _root; } + void normalize() { + if (_root) { + _root->normalize(); + } + } - /** - * Returns a const reference to the root of the trace tree. - * - * @return The root. - */ - const TraceNode &getRoot() const { return _root; } + void setStrict(bool strict) { + ensureRoot().setStrict(strict); + } + void addChild(TraceNode && child) { + ensureRoot().addChild(std::move(child)); + } + void addChild(Trace && child) { + if (!child.isEmpty()) { + addChild(std::move(*child._root)); + } + } + + bool isEmpty() const { return !_root || _root->isEmpty(); } + + uint32_t getNumChildren() const { return _root ? _root->getNumChildren() : 0; } + const TraceNode & getChild(uint32_t child) const { return getRoot().getChild(child); } + string encode() const; /** * Returns a string representation of the contained trace tree. This is a @@ -111,7 +108,16 @@ public: * * @return Readable trace string. */ - string toString() const { return _root.toString(31337); } + string toString(size_t limit=31337) const; + size_t computeMemoryUsage() const { + return _root ? _root->computeMemoryUsage() : 0; + } +private: + const TraceNode &getRoot() const { return *_root; } + TraceNode &ensureRoot(); + + std::unique_ptr<TraceNode> _root; + uint32_t _level; }; #define VESPALIB_TRACE2(ttrace, level, note, addTime) \ diff --git a/vespalib/src/vespa/vespalib/trace/tracenode.cpp b/vespalib/src/vespa/vespalib/trace/tracenode.cpp index d34b45025d3..12dd51ac677 100644 --- a/vespalib/src/vespa/vespalib/trace/tracenode.cpp +++ b/vespalib/src/vespa/vespalib/trace/tracenode.cpp @@ -40,22 +40,22 @@ struct Cmp { } // namespace <unnamed> -TraceNode::TraceNode() : - _parent(nullptr), - _strict(true), - _hasNote(false), - _note(""), - _children(), - _timestamp() +TraceNode::TraceNode() + : _note(""), + _children(), + _parent(nullptr), + _timestamp(), + _strict(true), + _hasNote(false) { } -TraceNode::TraceNode(const TraceNode &rhs) : - _parent(nullptr), - _strict(rhs._strict), - _hasNote(rhs._hasNote), - _note(rhs._note), - _children(), - _timestamp(rhs._timestamp) +TraceNode::TraceNode(const TraceNode &rhs) + : _note(rhs._note), + _children(), + _parent(nullptr), + _timestamp(rhs._timestamp), + _strict(rhs._strict), + _hasNote(rhs._hasNote) { addChildren(rhs._children); } @@ -65,22 +65,22 @@ TraceNode & TraceNode::operator =(const TraceNode &) = default; TraceNode::~TraceNode() = default; -TraceNode::TraceNode(const string ¬e, system_time timestamp) : - _parent(nullptr), - _strict(true), - _hasNote(true), - _note(note), - _children(), - _timestamp(timestamp) +TraceNode::TraceNode(const string ¬e, system_time timestamp) + : _note(note), + _children(), + _parent(nullptr), + _timestamp(timestamp), + _strict(true), + _hasNote(true) { } -TraceNode::TraceNode(system_time timestamp) : - _parent(nullptr), - _strict(true), - _hasNote(false), - _note(""), - _children(), - _timestamp(timestamp) +TraceNode::TraceNode(system_time timestamp) + : _note(""), + _children(), + _parent(nullptr), + _timestamp(timestamp), + _strict(true), + _hasNote(false) { } TraceNode & @@ -342,4 +342,17 @@ TraceNode::accept(TraceVisitor & visitor) const return visitor; } +size_t +TraceNode::computeMemoryUsage() const +{ + if (isLeaf()) { + return getNote().size(); + } + size_t childSum = 0; + for (const TraceNode & child : _children) { + childSum += child.computeMemoryUsage(); + } + return childSum; +} + } // namespace vespalib diff --git a/vespalib/src/vespa/vespalib/trace/tracenode.h b/vespalib/src/vespa/vespalib/trace/tracenode.h index 63e7bcd6dc0..7a7cdb89c69 100644 --- a/vespalib/src/vespa/vespalib/trace/tracenode.h +++ b/vespalib/src/vespa/vespalib/trace/tracenode.h @@ -23,12 +23,12 @@ struct TraceVisitor; */ class TraceNode { private: - TraceNode *_parent; - bool _strict; - bool _hasNote; string _note; std::vector<TraceNode> _children; + TraceNode *_parent; system_time _timestamp; + bool _strict; + bool _hasNote; public: /** @@ -253,6 +253,8 @@ public: */ TraceVisitor & accept(TraceVisitor & visitor) const; + size_t computeMemoryUsage() const; + }; } // namespace vespalib |