From ff21cb2da90860164e33d242d5197e4712fd6351 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 3 May 2016 10:07:14 +0200 Subject: [PATCH 1/2] optimizations --- arangod/Aql/AqlValue.cpp | 4 +- arangod/Utils/Transaction.cpp | 86 +++++++++++++++++++++++-- arangod/Utils/Transaction.h | 18 ++++++ arangod/VocBase/compactor.cpp | 6 +- arangod/VocBase/document-collection.cpp | 21 +++--- arangod/Wal/CollectorThread.cpp | 18 ++++-- lib/Basics/StringUtils.h | 2 +- 7 files changed, 129 insertions(+), 26 deletions(-) diff --git a/arangod/Aql/AqlValue.cpp b/arangod/Aql/AqlValue.cpp index eb0cd81c9a..30bf076258 100644 --- a/arangod/Aql/AqlValue.cpp +++ b/arangod/Aql/AqlValue.cpp @@ -356,7 +356,7 @@ AqlValue AqlValue::get(arangodb::AqlTransaction* trx, case VPACK_MANAGED: { VPackSlice s(slice()); if (s.isObject()) { - VPackSlice found(s.get(name)); + VPackSlice found(s.resolveExternal().get(name)); if (found.isCustom()) { // _id needs special treatment mustDestroy = true; @@ -399,7 +399,7 @@ AqlValue AqlValue::get(arangodb::AqlTransaction* trx, case VPACK_MANAGED: { VPackSlice s(slice()); if (s.isObject()) { - VPackSlice found(s.get(names)); + VPackSlice found(s.resolveExternal().get(names)); if (found.isCustom()) { // _id needs special treatment mustDestroy = true; diff --git a/arangod/Utils/Transaction.cpp b/arangod/Utils/Transaction.cpp index 24395089b6..db71b76016 100644 --- a/arangod/Utils/Transaction.cpp +++ b/arangod/Utils/Transaction.cpp @@ -736,8 +736,8 @@ VPackSlice Transaction::extractKeyFromDocument(VPackSlice const& slice) { // and point to the attribute value return VPackSlice(p + 1); } - // we actually should not get here. however, if for some reason we do, - // we simply fall back to the regular lookup method + + // fall back to the regular lookup method return slice.get(StaticStrings::KeyString); } @@ -768,8 +768,7 @@ VPackSlice Transaction::extractFromFromDocument(VPackSlice const& slice) { p += VPackSlice(p).byteSize(); } - // we actually should not get here. however, if for some reason we do, - // we simply fall back to the regular lookup method + // fall back to the regular lookup method return slice.get(StaticStrings::FromString); } @@ -800,11 +799,86 @@ VPackSlice Transaction::extractToFromDocument(VPackSlice const& slice) { p += VPackSlice(p).byteSize(); } - // we actually should not get here. however, if for some reason we do, - // we simply fall back to the regular lookup method + // fall back to the regular lookup method return slice.get(StaticStrings::ToString); } +////////////////////////////////////////////////////////////////////////////// +/// @brief quick access to the _rev attribute in a database document +/// the document must have at least three attributes: _key, _id, _rev +/// (possibly with _from and _to in between) +////////////////////////////////////////////////////////////////////////////// + +VPackSlice Transaction::extractRevFromDocument(VPackSlice const& slice) { + TRI_ASSERT(slice.isObject()); + TRI_ASSERT(slice.length() >= 2); + + uint8_t const* p = slice.begin() + slice.findDataOffset(slice.head()); + VPackValueLength count = 0; + + while (*p <= basics::VelocyPackHelper::ToAttribute && ++count <= 5) { + if (*p == basics::VelocyPackHelper::RevAttribute) { + // the + 1 is required so that we can skip over the attribute name + // and point to the attribute value + return VPackSlice(p + 1); + } + // skip over the attribute name + ++p; + // skip over the attribute value + p += VPackSlice(p).byteSize(); + } + + // fall back to the regular lookup method + return slice.get(StaticStrings::RevString); +} + +////////////////////////////////////////////////////////////////////////////// +/// @brief extract _key and _rev from a document, in one go +/// this is an optimized version used when loading collections, WAL +/// collection and compaction +////////////////////////////////////////////////////////////////////////////// + +void Transaction::extractKeyAndRevFromDocument(VPackSlice const& slice, + VPackSlice& keySlice, + TRI_voc_rid_t& revisionId) { + TRI_ASSERT(slice.isObject()); + TRI_ASSERT(slice.length() >= 2); + + uint8_t const* p = slice.begin() + slice.findDataOffset(slice.head()); + VPackValueLength count = 0; + bool foundKey = false; + bool foundRev = false; + + while (*p <= basics::VelocyPackHelper::ToAttribute && ++count <= 5) { + if (*p == basics::VelocyPackHelper::KeyAttribute) { + keySlice = VPackSlice(p + 1); + if (foundRev) { + return; + } + foundKey = true; + } else if (*p == basics::VelocyPackHelper::RevAttribute) { + VPackSlice revSlice(p + 1); + if (revSlice.isString()) { + revisionId = basics::StringUtils::uint64(revSlice.copyString()); + } else if (revSlice.isNumber()) { + revisionId = revSlice.getNumericValue(); + } + if (foundKey) { + return; + } + foundRev = true; + } + // skip over the attribute name + ++p; + // skip over the attribute value + p += VPackSlice(p).byteSize(); + } + + // fall back to regular lookup + keySlice = slice.get(StaticStrings::KeyString); + revisionId = basics::StringUtils::uint64(slice.get(StaticStrings::RevString).copyString()); +} + ////////////////////////////////////////////////////////////////////////////// /// @brief build a VPack object with _id, _key and _rev, the result is /// added to the builder in the argument as a single object. diff --git a/arangod/Utils/Transaction.h b/arangod/Utils/Transaction.h index a3a3ae5b19..3d34a198a9 100644 --- a/arangod/Utils/Transaction.h +++ b/arangod/Utils/Transaction.h @@ -306,6 +306,24 @@ class Transaction { ////////////////////////////////////////////////////////////////////////////// static VPackSlice extractToFromDocument(VPackSlice const&); + + ////////////////////////////////////////////////////////////////////////////// + /// @brief quick access to the _rev attribute in a database document + /// the document must have at least three attributes: _key, _id, _rev + /// (possibly with _from and _to in between) + ////////////////////////////////////////////////////////////////////////////// + + static VPackSlice extractRevFromDocument(VPackSlice const&); + + ////////////////////////////////////////////////////////////////////////////// + /// @brief extract _key and _rev from a document, in one go + /// this is an optimized version used when loading collections, WAL + /// collection and compaction + ////////////////////////////////////////////////////////////////////////////// + + static void extractKeyAndRevFromDocument(VPackSlice const& slice, + VPackSlice& keySlice, + TRI_voc_rid_t& revisionId); ////////////////////////////////////////////////////////////////////////////// /// @brief read any (random) document diff --git a/arangod/VocBase/compactor.cpp b/arangod/VocBase/compactor.cpp index 53b5741413..4446c29618 100644 --- a/arangod/VocBase/compactor.cpp +++ b/arangod/VocBase/compactor.cpp @@ -392,8 +392,10 @@ static bool Compactifier(TRI_df_marker_t const* marker, void* data, if (type == TRI_DF_MARKER_VPACK_DOCUMENT) { VPackSlice const slice(reinterpret_cast(marker) + DatafileHelper::VPackOffset(type)); TRI_ASSERT(slice.isObject()); - VPackSlice const keySlice(Transaction::extractKeyFromDocument(slice)); - TRI_voc_rid_t const rid = std::stoull(slice.get(StaticStrings::RevString).copyString()); + + VPackSlice keySlice; + TRI_voc_rid_t rid = 0; + Transaction::extractKeyAndRevFromDocument(slice, keySlice, rid); // check if the document is still active auto primaryIndex = document->primaryIndex(); diff --git a/arangod/VocBase/document-collection.cpp b/arangod/VocBase/document-collection.cpp index db3285ac75..43b6e34627 100644 --- a/arangod/VocBase/document-collection.cpp +++ b/arangod/VocBase/document-collection.cpp @@ -786,10 +786,12 @@ static int OpenIteratorHandleDocumentMarker(TRI_df_marker_t const* marker, arangodb::Transaction* trx = state->_trx; VPackSlice const slice(reinterpret_cast(marker) + DatafileHelper::VPackOffset(TRI_DF_MARKER_VPACK_DOCUMENT)); - VPackSlice const keySlice = Transaction::extractKeyFromDocument(slice); - TRI_voc_rid_t const rid = StringUtils::uint64(slice.get(StaticStrings::RevString).copyString()); + VPackSlice keySlice; + TRI_voc_rid_t revisionId; + + Transaction::extractKeyAndRevFromDocument(slice, keySlice, revisionId); - SetRevision(document, rid, false); + SetRevision(document, revisionId, false); document->_keyGenerator->track(keySlice.copyString()); ++state->_documents; @@ -837,8 +839,8 @@ static int OpenIteratorHandleDocumentMarker(TRI_df_marker_t const* marker, } // it is an update, but only if found has a smaller revision identifier - else if (found->revisionId() < rid || - (found->revisionId() == rid && found->getFid() <= fid)) { + else if (found->revisionId() < revisionId || + (found->revisionId() == revisionId && found->getFid() <= fid)) { // save the old data TRI_doc_mptr_t oldData = *found; @@ -889,10 +891,13 @@ static int OpenIteratorHandleDeletionMarker(TRI_df_marker_t const* marker, arangodb::Transaction* trx = state->_trx; VPackSlice const slice(reinterpret_cast(marker) + DatafileHelper::VPackOffset(TRI_DF_MARKER_VPACK_REMOVE)); - VPackSlice const keySlice = Transaction::extractKeyFromDocument(slice); - TRI_voc_rid_t const rid = StringUtils::uint64(slice.get(StaticStrings::RevString).copyString()); + + VPackSlice keySlice; + TRI_voc_rid_t revisionId; + + Transaction::extractKeyAndRevFromDocument(slice, keySlice, revisionId); - document->setLastRevision(rid, false); + document->setLastRevision(revisionId, false); document->_keyGenerator->track(keySlice.copyString()); ++state->_deletions; diff --git a/arangod/Wal/CollectorThread.cpp b/arangod/Wal/CollectorThread.cpp index 6f65bf4a0d..cc1a3947bb 100644 --- a/arangod/Wal/CollectorThread.cpp +++ b/arangod/Wal/CollectorThread.cpp @@ -589,10 +589,12 @@ void CollectorThread::processCollectionMarker( VPackSlice slice(reinterpret_cast(walMarker) + DatafileHelper::VPackOffset(type)); TRI_ASSERT(slice.isObject()); - - TRI_voc_rid_t revisionId = arangodb::basics::VelocyPackHelper::stringUInt64(slice.get(StaticStrings::RevString)); - - auto found = document->primaryIndex()->lookupKey(&trx, Transaction::extractKeyFromDocument(slice)); + + VPackSlice keySlice; + TRI_voc_rid_t revisionId = 0; + Transaction::extractKeyAndRevFromDocument(slice, keySlice, revisionId); + + auto found = document->primaryIndex()->lookupKey(&trx, keySlice); if (found == nullptr || found->revisionId() != revisionId || found->getMarkerPtr() != walMarker) { @@ -615,10 +617,12 @@ void CollectorThread::processCollectionMarker( VPackSlice slice(reinterpret_cast(walMarker) + DatafileHelper::VPackOffset(type)); TRI_ASSERT(slice.isObject()); + + VPackSlice keySlice; + TRI_voc_rid_t revisionId = 0; + Transaction::extractKeyAndRevFromDocument(slice, keySlice, revisionId); - TRI_voc_rid_t revisionId = arangodb::basics::VelocyPackHelper::stringUInt64(slice.get(StaticStrings::RevString)); - - auto found = document->primaryIndex()->lookupKey(&trx, Transaction::extractKeyFromDocument(slice)); + auto found = document->primaryIndex()->lookupKey(&trx, keySlice); if (found != nullptr && found->revisionId() > revisionId) { // somebody re-created the document with a newer revision diff --git a/lib/Basics/StringUtils.h b/lib/Basics/StringUtils.h index b24260fb1d..9564df81ad 100644 --- a/lib/Basics/StringUtils.h +++ b/lib/Basics/StringUtils.h @@ -436,7 +436,7 @@ inline int64_t int64_check(char const* value, size_t size) { uint64_t uint64(std::string const& str); -inline int64_t uint64(char const* value, size_t size) { +inline uint64_t uint64(char const* value, size_t size) { return StringUtils::uint64(std::string(value, size)); } From eb788e179118290ff61273a1b6ff4c5c76e8f3e0 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 3 May 2016 10:42:17 +0200 Subject: [PATCH 2/2] fixed tests --- 3rdParty/velocypack/include/velocypack/Slice.h | 10 +++++++++- arangod/Aql/AqlValue.cpp | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/3rdParty/velocypack/include/velocypack/Slice.h b/3rdParty/velocypack/include/velocypack/Slice.h index 9497bb71b6..75ba73397d 100644 --- a/3rdParty/velocypack/include/velocypack/Slice.h +++ b/3rdParty/velocypack/include/velocypack/Slice.h @@ -365,7 +365,8 @@ class Slice { // look for the specified attribute path inside an Object // returns a Slice(ValueType::None) if not found - Slice get(std::vector const& attributes) const { + Slice get(std::vector const& attributes, + bool resolveExternals = false) const { size_t const n = attributes.size(); if (n == 0) { throw Exception(Exception::InvalidAttributePath); @@ -373,11 +374,18 @@ class Slice { // use ourselves as the starting point Slice last = Slice(start()); + if (resolveExternals) { + last = last.resolveExternal(); + } for (size_t i = 0; i < attributes.size(); ++i) { // fetch subattribute last = last.get(attributes[i]); // abort as early as possible + if (last.isExternal()) { + last = last.resolveExternal(); + } + if (last.isNone() || (i + 1 < n && !last.isObject())) { return Slice(); } diff --git a/arangod/Aql/AqlValue.cpp b/arangod/Aql/AqlValue.cpp index d4f485c33e..e03862c092 100644 --- a/arangod/Aql/AqlValue.cpp +++ b/arangod/Aql/AqlValue.cpp @@ -399,7 +399,7 @@ AqlValue AqlValue::get(arangodb::AqlTransaction* trx, case VPACK_MANAGED: { VPackSlice s(slice()); if (s.isObject()) { - VPackSlice found(s.resolveExternal().get(names)); + VPackSlice found(s.resolveExternal().get(names, true)); if (found.isCustom()) { // _id needs special treatment mustDestroy = true;