diff options
Diffstat (limited to 'src')
10 files changed, 125 insertions, 79 deletions
diff --git a/src/mongo/db/exec/document_value/document.cpp b/src/mongo/db/exec/document_value/document.cpp index 38fc2b8a758..4e8b0a1a7b1 100644 --- a/src/mongo/db/exec/document_value/document.cpp +++ b/src/mongo/db/exec/document_value/document.cpp @@ -373,6 +373,7 @@ intrusive_ptr<DocumentStorage> DocumentStorage::clone() const { out->_haveLazyLoadedMetadata = _haveLazyLoadedMetadata; out->_metadataFields = _metadataFields; + out->_snapshottedSize = _snapshottedSize; return out; } @@ -394,6 +395,7 @@ void DocumentStorage::reset(const BSONObj& bson, bool stripMetadata) { _numBytesFromBSONInCache = 0; _stripMetadata = stripMetadata; _modified = false; + _snapshottedSize = 0; // Clean cache. for (auto it = iteratorCacheOnly(); !it.atEnd(); it.advance()) { @@ -704,31 +706,17 @@ Value Document::getNestedField(const FieldPath& path, vector<Position>* position return getNestedFieldHelper(*this, path, positions, 0); } -size_t Document::getApproximateSizeWithoutBackingBSON() const { - size_t size = sizeof(Document); - if (!_storage) - return size; - - size += sizeof(DocumentStorage); - size += storage().allocatedBytes(); - - for (auto it = storage().iteratorCacheOnly(); !it.atEnd(); it.advance()) { - size += it->val.getApproximateSize(); - size -= sizeof(Value); // already accounted for above - } - - // The metadata also occupies space in the document storage that's pre-allocated. - size += storage().getMetadataApproximateSize(); - - return size; +size_t Document::getApproximateSize() const { + return sizeof(Document) + storage().snapshottedApproximateSize(); } -size_t Document::getApproximateSize() const { - return getApproximateSizeWithoutBackingBSON() + storage().bsonObjSize(); +size_t Document::getCurrentApproximateSize() const { + return sizeof(Document) + storage().currentApproximateSize(); } size_t Document::memUsageForSorter() const { - return getApproximateSizeWithoutBackingBSON() + storage().nonCachedBsonObjSize(); + return storage().currentApproximateSize() - storage().bsonObjSize() + + storage().nonCachedBsonObjSize(); } void Document::hash_combine(size_t& seed, diff --git a/src/mongo/db/exec/document_value/document.h b/src/mongo/db/exec/document_value/document.h index 81b87595ed6..641a89f099e 100644 --- a/src/mongo/db/exec/document_value/document.h +++ b/src/mongo/db/exec/document_value/document.h @@ -190,7 +190,8 @@ public: /** * Get the approximate size of the Document, plus its underlying storage and sub-values. Returns - * size in bytes. + * size in bytes. The return value of this function is snapshotted. All subsequent calls of this + * method will return the same value. * * Note: Some memory may be shared with other Documents or between fields within a single * Document so this can overestimate usage. @@ -201,6 +202,11 @@ public: size_t getApproximateSize() const; /** + * Same as 'getApproximateSize()', but this method re-computes the size on every call. + */ + size_t getCurrentApproximateSize() const; + + /** * Return the approximate amount of space used by metadata. */ size_t getMetadataApproximateSize() const { @@ -369,12 +375,6 @@ private: getNestedFieldNonCachingHelper(const FieldPath& dottedField, size_t level) const; boost::intrusive_ptr<const DocumentStorage> _storage; - - /** - * Returns the approximate size of this `Document` instance without considering the size of its - * backing BSON object. - */ - size_t getApproximateSizeWithoutBackingBSON() const; }; // @@ -653,6 +653,7 @@ public: * TODO: there are some optimizations that may make sense at freeze time. */ Document freeze() { + resetSnapshottedApproximateSize(); // This essentially moves _storage into a new Document by way of temp. Document ret; boost::intrusive_ptr<const DocumentStorage> temp(storagePtr(), /*inc_ref_count=*/false); @@ -671,8 +672,12 @@ public: * Note that unlike freeze(), this indicates intention to continue * modifying this document. The returned Document will not observe * future changes to this MutableDocument. + * + * Note that the computed snapshotted approximate size of the Document + * is not preserved across calls. */ Document peek() { + resetSnapshottedApproximateSize(); return Document(storagePtr()); } @@ -738,12 +743,19 @@ private: MutableValue getNestedFieldHelper(const FieldPath& dottedField, size_t level); MutableValue getNestedFieldHelper(const std::vector<Position>& positions, size_t level); - // this should only be called by storage methods and peek/freeze + // this should only be called by storage methods and peek/freeze/resetsnapshottedApproximateSize const DocumentStorage* storagePtr() const { dassert(!_storage || typeid(*_storage) == typeid(const DocumentStorage)); return static_cast<const DocumentStorage*>(_storage); } + void resetSnapshottedApproximateSize() { + auto mutableStorage = const_cast<DocumentStorage*>(storagePtr()); + if (mutableStorage) { + mutableStorage->resetSnapshottedApproximateSize(); + } + } + // These are both const to prevent modifications bypassing storage() method. // They always point to NULL or an object with dynamic type DocumentStorage. const RefCountable* _storageHolder; // Only used in constructors and destructor diff --git a/src/mongo/db/exec/document_value/document_internal.h b/src/mongo/db/exec/document_value/document_internal.h index 48a9135500c..ed9dd1285c2 100644 --- a/src/mongo/db/exec/document_value/document_internal.h +++ b/src/mongo/db/exec/document_value/document_internal.h @@ -603,6 +603,29 @@ public: return _bson; } + size_t currentApproximateSize() const { + size_t size = sizeof(DocumentStorage) + allocatedBytes() + getMetadataApproximateSize() + + bsonObjSize(); + + for (auto it = iteratorCacheOnly(); !it.atEnd(); it.advance()) { + size += it->val.getApproximateSize() - sizeof(Value); + } + + return size; + } + + size_t snapshottedApproximateSize() const { + if (_snapshottedSize == 0) { + const_cast<DocumentStorage*>(this)->_snapshottedSize = currentApproximateSize(); + } + + return _snapshottedSize; + } + + void resetSnapshottedApproximateSize() { + _snapshottedSize = 0; + } + private: /// Returns the position of the named field in the cache or Position() template <typename T> @@ -701,6 +724,8 @@ private: // a conversion to BSON; i.e. if there are not any modifications we can directly return _bson. bool _modified{false}; + size_t _snapshottedSize{0}; + // Defined in document.cpp static const DocumentStorage kEmptyDoc; diff --git a/src/mongo/db/exec/document_value/document_value_test.cpp b/src/mongo/db/exec/document_value/document_value_test.cpp index aee0844b164..b9f92debf29 100644 --- a/src/mongo/db/exec/document_value/document_value_test.cpp +++ b/src/mongo/db/exec/document_value/document_value_test.cpp @@ -357,6 +357,46 @@ TEST(DocumentGetFieldNonCaching, TraverseArray) { checkArrayTagIsReturned(); } +TEST(DocumentSize, ApproximateSizeIsSnapshotted) { + const auto rawBson = BSON("field" + << "value"); + const Document document{rawBson}; + const auto noCacheSize = document.getApproximateSize(); + + // Force the cache construction, making the total size of the 'Document' bigger. + // 'getApproximateSize()' must still return the same value. + document.fillCache(); + const auto fullCacheSizeSnapshot = document.getApproximateSize(); + const auto fullCacheSizeCurrent = document.getCurrentApproximateSize(); + ASSERT_EQ(noCacheSize, fullCacheSizeSnapshot); + ASSERT_LT(noCacheSize, fullCacheSizeCurrent); +} + +TEST(DocumentSize, ApproximateSizeDuringBuildIsUpdated) { + MutableDocument builder; + builder.addField("a1", Value(1)); + builder.addField("a2", mongo::Value(2)); + builder.addField("a3", mongo::Value(3)); + auto middleBuildSize = builder.getApproximateSize(); + + builder.addField("a4", Value(4)); + builder.addField("a5", mongo::Value(5)); + builder.addField("a6", mongo::Value(6)); + auto peekSize = builder.peek().getApproximateSize(); + + builder.addField("a7", Value(7)); + builder.addField("a8", mongo::Value(8)); + builder.addField("a9", mongo::Value(9)); + auto beforeFreezeSize = builder.getApproximateSize(); + + Document result = builder.freeze(); + auto frozenSize = result.getApproximateSize(); + + ASSERT_LT(middleBuildSize, peekSize); + ASSERT_LT(peekSize, beforeFreezeSize); + ASSERT_EQ(beforeFreezeSize, frozenSize); +} + /** Add Document fields. */ class AddField { public: diff --git a/src/mongo/db/pipeline/accumulator_multi.cpp b/src/mongo/db/pipeline/accumulator_multi.cpp index 4f63aaa4149..3dd7f02ce51 100644 --- a/src/mongo/db/pipeline/accumulator_multi.cpp +++ b/src/mongo/db/pipeline/accumulator_multi.cpp @@ -599,12 +599,6 @@ void AccumulatorTopBottomN<sense, single>::_processValue(const Value& val) { } } - // TODO SERVER-61281 consider removing this call to fillCache(). - // Since Document caches fields the size of this cache and getApproximateSize() can vary - // depending on access. In order to avoid this and make sure we subtract the right amount if - // remove() ever gets called, we can fill the cache to get a consistent view of the size. - // Normally the outer window function code handles this, but _genKeyOutPair() makes a new - // document for sortKey, so its cache get reset. keyOutPair.first.fillCache(); const auto memUsage = keyOutPair.first.getApproximateSize() + keyOutPair.second.getApproximateSize() + sizeof(KeyOutPair); @@ -626,9 +620,6 @@ void AccumulatorTopBottomN<sense, single>::remove(const Value& val) { auto it = _map->lower_bound(keyOutPair.first); _map->erase(it); - // TODO SERVER-61281 consider removing this comment if its no longer relevant. - // After calling lower_bound() it uses SortKeyComparator and the sortKey's field cache should be - // fully populated so no need to call fillCache() again. _memUsageBytes -= keyOutPair.first.getApproximateSize() + keyOutPair.second.getApproximateSize() + sizeof(KeyOutPair); } diff --git a/src/mongo/db/pipeline/lookup_set_cache_test.cpp b/src/mongo/db/pipeline/lookup_set_cache_test.cpp index 6670b5b5971..ec78b0b0572 100644 --- a/src/mongo/db/pipeline/lookup_set_cache_test.cpp +++ b/src/mongo/db/pipeline/lookup_set_cache_test.cpp @@ -214,7 +214,7 @@ TEST(LookupSetCacheTest, DocumentWithStorageCachePopulated) { // initialization. BSONObj input = BSON("a" << 1); const auto doc1 = Document(input); - const auto sizeOfDoc1Before = doc1.getApproximateSize(); + const auto sizeOfDoc1Before = doc1.getCurrentApproximateSize(); auto key = Value("foo"_sd); // Insert a cache entry and verify that both the key and the document are accounted for in the @@ -228,14 +228,14 @@ TEST(LookupSetCacheTest, DocumentWithStorageCachePopulated) { auto prevCacheSize = cache.getMemoryUsage(); const auto doc2 = Document({{"a", 2}}); cache.insert(key, doc2); - ASSERT_EQ(cache.getMemoryUsage(), prevCacheSize + doc2.getApproximateSize()); + ASSERT_EQ(cache.getMemoryUsage(), prevCacheSize + doc2.getCurrentApproximateSize()); // Calling serializeForSorter() should grow the overall document size. Verify that growing the // size of the 'Document' object does not have impact on the size stored in 'cache'. prevCacheSize = cache.getMemoryUsage(); BufBuilder builder; doc1.serializeForSorter(builder); - ASSERT_LT(sizeOfDoc1Before, doc1.getApproximateSize()); + ASSERT_LT(sizeOfDoc1Before, doc1.getCurrentApproximateSize()); ASSERT_EQ(prevCacheSize, cache.getMemoryUsage()); cache.evictOne(); diff --git a/src/mongo/db/pipeline/memory_usage_tracker.h b/src/mongo/db/pipeline/memory_usage_tracker.h index 9847b41e3cb..6887e91289a 100644 --- a/src/mongo/db/pipeline/memory_usage_tracker.h +++ b/src/mongo/db/pipeline/memory_usage_tracker.h @@ -50,13 +50,11 @@ public: PerFunctionMemoryTracker() = delete; void update(long long diff) { - - // TODO SERVER-61281: this is a temporary measure in tackling the problem in this - // ticket. It prevents the underflow from happening but doesn't address the cause - // which is inaccurate tracking. Once inaccurate tracking is resolved, the underflow - // assertion could be restored. See SERVER-62856 and SERVER-65473 for details. - - set(std::max(_currentMemoryBytes + diff, 0LL)); + tassert(5578603, + str::stream() << "Underflow on memory tracking, attempting to add " << diff + << " but only " << _currentMemoryBytes << " available", + diff >= 0 || _currentMemoryBytes >= std::abs(diff)); + set(_currentMemoryBytes + diff); } void set(long long total) { @@ -147,13 +145,11 @@ public: * Updates total memory usage. */ void update(long long diff) { - - // TODO SERVER-61281: this is a temporary measure in tackling the problem in this - // ticket. It prevents the underflow from happening but doesn't address the cause - // which is inaccurate tracking. Once inaccurate tracking is resolved, the underflow - // assertion could be restored. See SERVER-62856 and SERVER-65473 for details. - - set(std::max(_memoryUsageBytes + diff, 0LL)); + tassert(5578602, + str::stream() << "Underflow on memory tracking, attempting to add " << diff + << " but only " << _memoryUsageBytes << " available", + diff >= 0 || (int)_memoryUsageBytes >= -1 * diff); + set(_memoryUsageBytes + diff); } auto currentMemoryBytes() const { diff --git a/src/mongo/db/pipeline/memory_usage_tracker_test.cpp b/src/mongo/db/pipeline/memory_usage_tracker_test.cpp index 81ca06a1381..9d383a8a468 100644 --- a/src/mongo/db/pipeline/memory_usage_tracker_test.cpp +++ b/src/mongo/db/pipeline/memory_usage_tracker_test.cpp @@ -99,36 +99,26 @@ TEST_F(MemoryUsageTrackerTest, UpdateUsageUpdatesGlobal) { ASSERT_EQ(_tracker.maxMemoryBytes(), 150LL); } -// TODO SERVER-61281: Need to add back the DEATH_TEST_F removed in SERVER-65473, once -// MemoryUsageTracker.update() no longer underflows. - -TEST_F(MemoryUsageTrackerTest, UpdateFunctionUsageToNegativeIsDisallowed) { +DEATH_TEST_F(MemoryUsageTrackerTest, + UpdateFunctionUsageToNegativeIsDisallowed, + "Underflow on memory tracking") { _funcTracker.set(50LL); ASSERT_EQ(_funcTracker.currentMemoryBytes(), 50LL); ASSERT_EQ(_funcTracker.maxMemoryBytes(), 50LL); ASSERT_EQ(_tracker.currentMemoryBytes(), 50LL); ASSERT_EQ(_tracker.maxMemoryBytes(), 50LL); - // TODO SERVER-61281: This should throw an assertion once accurate tracking is implemented and - // no underflow should happen. That assertion would make sure "Underflow in memory tracking" - // is reported. - _funcTracker.update(-100); - ASSERT_EQ(_funcTracker.currentMemoryBytes(), 0LL); - ASSERT_EQ(_tracker.currentMemoryBytes(), 0LL); } -TEST_F(MemoryUsageTrackerTest, UpdateMemUsageToNegativeIsDisallowed) { +DEATH_TEST_F(MemoryUsageTrackerTest, + UpdateMemUsageToNegativeIsDisallowed, + "Underflow on memory tracking") { _tracker.set(50LL); ASSERT_EQ(_tracker.currentMemoryBytes(), 50LL); ASSERT_EQ(_tracker.maxMemoryBytes(), 50LL); - // TODO SERVER-61281: This should throw an assertion once accurate tracking is implemented and - // no underflow should happen. That assertion would make sure "Underflow in memory tracking" - // is reported. - _tracker.update(-100); - ASSERT_EQ(_tracker.currentMemoryBytes(), 0LL); } } // namespace diff --git a/src/mongo/db/pipeline/window_function/partition_iterator_test.cpp b/src/mongo/db/pipeline/window_function/partition_iterator_test.cpp index cb6b5bcf1bf..4ba0a692e47 100644 --- a/src/mongo/db/pipeline/window_function/partition_iterator_test.cpp +++ b/src/mongo/db/pipeline/window_function/partition_iterator_test.cpp @@ -499,7 +499,7 @@ TEST_F(PartitionIteratorTest, MemoryUsageAccountsForDocumentIteratorCache) { const auto mock = DocumentSourceMock::createForTest(docs, getExpCtx()); [[maybe_unused]] auto accessor = makeDefaultAccessor(mock, boost::none); - size_t initialDocSize = docs[0].getDocument().getApproximateSize(); + size_t initialDocSize = docs[0].getDocument().getCurrentApproximateSize(); // Pull in the first document, and verify the reported size of the iterator is roughly double // the size of the document. The size of the iterator is double the size of the document because @@ -525,7 +525,7 @@ TEST_F(PartitionIteratorTest, MemoryUsageAccountsForArraysInDocumentIteratorCach const auto mock = DocumentSourceMock::createForTest(docs, getExpCtx()); [[maybe_unused]] auto accessor = makeDefaultAccessor(mock, boost::none); - size_t initialDocSize = docs[0].getDocument().getApproximateSize(); + size_t initialDocSize = docs[0].getDocument().getCurrentApproximateSize(); // Pull in the first document, and verify the reported size of the iterator is roughly // triple the size of the document. The reason for this is that 'largeStr' is cached twice; once @@ -550,7 +550,7 @@ TEST_F(PartitionIteratorTest, MemoryUsageAccountsForNestedArraysInDocumentIterat const auto mock = DocumentSourceMock::createForTest(docs, getExpCtx()); [[maybe_unused]] auto accessor = makeDefaultAccessor(mock, boost::none); - size_t initialDocSize = docs[0].getDocument().getApproximateSize(); + size_t initialDocSize = docs[0].getDocument().getCurrentApproximateSize(); // Pull in the first document, and verify the reported size of the iterator is roughly // triple the size of the document. The reason for this is that 'largeStr' is cached twice; once @@ -575,7 +575,7 @@ TEST_F(PartitionIteratorTest, MemoryUsageAccountsForNestedObjInDocumentIteratorC const auto mock = DocumentSourceMock::createForTest(docs, getExpCtx()); [[maybe_unused]] auto accessor = makeDefaultAccessor(mock, boost::none); - size_t initialDocSize = docs[0].getDocument().getApproximateSize(); + size_t initialDocSize = docs[0].getDocument().getCurrentApproximateSize(); // Pull in the first document, and verify the reported size. TODO SERVER-57011: The approximate // size should not double count the nested strings. @@ -592,7 +592,7 @@ TEST_F(PartitionIteratorTest, MemoryUsageAccountsForReleasedDocuments) { const auto mock = DocumentSourceMock::createForTest(docs, getExpCtx()); auto accessor = makeDefaultAccessor(mock, boost::none); - size_t initialDocSize = docs[0].getDocument().getApproximateSize(); + size_t initialDocSize = docs[0].getDocument().getCurrentApproximateSize(); // Pull in the first document, and verify the reported size of the iterator is roughly double // the size of the document. diff --git a/src/mongo/db/pipeline/window_function/window_function_top_bottom_n.h b/src/mongo/db/pipeline/window_function/window_function_top_bottom_n.h index fbf77c59fa2..64d3319efb9 100644 --- a/src/mongo/db/pipeline/window_function/window_function_top_bottom_n.h +++ b/src/mongo/db/pipeline/window_function/window_function_top_bottom_n.h @@ -56,17 +56,17 @@ public: explicit WindowFunctionTopBottomN(ExpressionContext* const expCtx, SortPattern sp, long long n) : WindowFunctionState(expCtx), _acc(expCtx, std::move(sp), true) { _acc.startNewGroup(Value(n)); - _memUsageBytes = sizeof(*this); + updateMemUsage(); } void add(Value value) final { _acc.process(value, false); - _memUsageBytes = _acc.getMemUsage(); + updateMemUsage(); } void remove(Value value) final { _acc.remove(value); - _memUsageBytes = _acc.getMemUsage(); + updateMemUsage(); } Value getValue() const final { @@ -75,10 +75,14 @@ public: void reset() final { _acc.reset(); - _memUsageBytes = _acc.getMemUsage(); + updateMemUsage(); } private: + void updateMemUsage() { + _memUsageBytes = sizeof(*this) + _acc.getMemUsage(); + } + AccumulatorTopBottomN<sense, single> _acc; }; |