diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index ec6f0fab2c..599c661b23 100644 --- a/arangod/Aql/Ast.cpp +++ b/arangod/Aql/Ast.cpp @@ -3802,13 +3802,8 @@ AstNode* Ast::createNode(AstNodeType type) { auto node = new AstNode(type); - try { - // register the node so it gets freed automatically later - _query->addNode(node); - } catch (...) { - delete node; - throw; - } + // register the node so it gets freed automatically later + _query->addNode(node); return node; } diff --git a/arangod/Aql/QueryResources.cpp b/arangod/Aql/QueryResources.cpp index 041d7d76a0..292e5c5cfa 100644 --- a/arangod/Aql/QueryResources.cpp +++ b/arangod/Aql/QueryResources.cpp @@ -37,7 +37,9 @@ static char const* EmptyString = ""; QueryResources::QueryResources(ResourceMonitor* resourceMonitor) : _resourceMonitor(resourceMonitor), +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE _stringsLength(0), +#endif _shortStringStorage(_resourceMonitor, 1024) {} QueryResources::~QueryResources() { @@ -51,7 +53,7 @@ QueryResources::~QueryResources() { delete it; } -#ifdef ARANGODB_USE_MAINTAINER_MODE +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE // we are in the destructor here already. decreasing the memory usage counters will only // provide a benefit (in terms of assertions) if we are in maintainer mode, so we can // save all these operations in non-maintainer mode @@ -60,14 +62,13 @@ QueryResources::~QueryResources() { #endif } -void QueryResources::steal() { - // we are not responsible for freeing any data, so we delete our inventory - _strings.clear(); - _nodes.clear(); -} - /// @brief add a node to the list of nodes void QueryResources::addNode(AstNode* node) { + auto guard = scopeGuard([node] () { + // in case something goes wrong, we must free the node we got to prevent memleaks + delete node; + }); + size_t capacity; if (_nodes.empty()) { @@ -99,6 +100,9 @@ void QueryResources::addNode(AstNode* node) { // will not fail _nodes.emplace_back(node); + + // safely took over the ownership for the node, cancel the deletion now + guard.cancel(); } /// @brief register a string @@ -143,46 +147,55 @@ char* QueryResources::registerEscapedString(char const* p, size_t length, return registerLongString(copy, outLength); } +/// @brief registers a long string and takes over the ownership for it char* QueryResources::registerLongString(char* copy, size_t length) { if (copy == nullptr) { THROW_ARANGO_EXCEPTION(TRI_ERROR_OUT_OF_MEMORY); } - try { - size_t capacity; - - if (_strings.empty()) { - // reserve some initial space for string storage - capacity = 8; - } else { - capacity = _strings.size() + 1; - if (capacity > _strings.capacity()) { - capacity *= 2; - } - } - - TRI_ASSERT(capacity > _strings.size()); - - // reserve space - if (capacity > _strings.capacity()) { - _resourceMonitor->increaseMemoryUsage(((capacity - _strings.size()) * sizeof(char*)) + length); - try { - _strings.reserve(capacity); - } catch (...) { - // revert change in memory increase - _resourceMonitor->decreaseMemoryUsage(((capacity - _strings.size()) * sizeof(char*)) + length); - throw; - } - } - - // will not fail - _strings.emplace_back(copy); - _stringsLength += length; - - return copy; - } catch (...) { - // prevent memleak + auto guard = scopeGuard([copy] () { + // in case something goes wrong, we must free the string we got to prevent memleaks TRI_FreeString(copy); - throw; + }); + + size_t capacity; + + if (_strings.empty()) { + // reserve some initial space for string storage + capacity = 8; + } else { + capacity = _strings.size() + 1; + if (capacity > _strings.capacity()) { + capacity *= 2; + } } + + TRI_ASSERT(capacity > _strings.size()); + + // reserve space + if (capacity > _strings.capacity()) { + // not enough capacity... + _resourceMonitor->increaseMemoryUsage(((capacity - _strings.size()) * sizeof(char*)) + length); + try { + _strings.reserve(capacity); + } catch (...) { + // revert change in memory increase + _resourceMonitor->decreaseMemoryUsage(((capacity - _strings.size()) * sizeof(char*)) + length); + throw; + } + } else { + // got enough capacity for the new string + _resourceMonitor->increaseMemoryUsage(length); + } + + // will not fail + _strings.emplace_back(copy); +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE + _stringsLength += length; +#endif + + // safely took over the ownership fo the string, cancel the deletion now + guard.cancel(); + + return copy; } diff --git a/arangod/Aql/QueryResources.h b/arangod/Aql/QueryResources.h index eb08e3dacd..e12b395398 100644 --- a/arangod/Aql/QueryResources.h +++ b/arangod/Aql/QueryResources.h @@ -42,8 +42,6 @@ class QueryResources { explicit QueryResources(ResourceMonitor*); ~QueryResources(); - void steal(); - /// @brief add a node to the list of nodes void addNode(AstNode*); @@ -62,6 +60,7 @@ class QueryResources { char* registerEscapedString(char const* p, size_t length, size_t& outLength); private: + /// @brief registers a long string and takes over the ownership for it char* registerLongString(char* copy, size_t length); private: @@ -74,7 +73,9 @@ class QueryResources { std::vector _strings; /// @brief cumulated length of strings in _strings +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE size_t _stringsLength; +#endif /// @brief short string storage. uses less memory allocations for short /// strings diff --git a/arangod/Aql/ShortStringStorage.cpp b/arangod/Aql/ShortStringStorage.cpp index e3fe90234f..4bf2f0851f 100644 --- a/arangod/Aql/ShortStringStorage.cpp +++ b/arangod/Aql/ShortStringStorage.cpp @@ -30,7 +30,10 @@ using namespace arangodb::aql; /// @brief create a short string storage instance ShortStringStorage::ShortStringStorage(ResourceMonitor* resourceMonitor, size_t blockSize) - : _resourceMonitor(resourceMonitor), _blocks(), _blockSize(blockSize), _current(nullptr), _end(nullptr) { + : _resourceMonitor(resourceMonitor), + _blockSize(blockSize), + _current(nullptr), + _end(nullptr) { TRI_ASSERT(blockSize >= 64); } diff --git a/lib/Basics/FixedSizeAllocator.h b/lib/Basics/FixedSizeAllocator.h index 0aa51b235d..8a1bfdf2bc 100644 --- a/lib/Basics/FixedSizeAllocator.h +++ b/lib/Basics/FixedSizeAllocator.h @@ -61,7 +61,7 @@ class FixedSizeAllocator { if (this != &other) { TRI_ASSERT(_itemSize == other._itemSize); - delete [] _alloc; + delete[] _alloc; _nrAlloc = other._nrAlloc; _nrUsed = other._nrUsed; _alloc = other._alloc; @@ -155,8 +155,7 @@ class FixedSizeAllocator { void allocateBlock() { size_t const size = 128 << (std::min)(size_t(8), _blocks.size()); auto block = std::make_unique(_itemSize, size); - _blocks.emplace_back(block.get()); - block.release(); + _blocks.emplace_back(std::move(block)); } std::vector> _blocks;