Merge pull request #11764 from vespa-engine/balder/log-max-bucketizerguard-holdtime-in-us

Balder/log max bucketizerguard holdtime in us

11 files changed, 48 insertions, 38 deletions

diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp
index 6165e94715c..6abbf477959 100644
--- a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp
+++ b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.cpp
@@ -31,7 +31,6 @@ using vespalib::compression::CompressionConfig;
 using search::DocumentStore;
 using search::IDocumentStore;
 using search::LogDocumentStore;
-using search::LogDataStore;
 using search::WriteableFileChunk;
 using vespalib::makeLambdaTask;
 
@@ -83,14 +82,14 @@ ShrinkSummaryLidSpaceFlushTarget::initFlush(SerialNum currentSerial)
 SummaryManager::SummarySetup::
 SummarySetup(const vespalib::string & baseDir, const DocTypeName & docTypeName, const SummaryConfig & summaryCfg,
              const SummarymapConfig & summarymapCfg, const JuniperrcConfig & juniperCfg,
-             const search::IAttributeManager::SP &attributeMgr, const search::IDocumentStore::SP & docStore,
-             const std::shared_ptr<const DocumentTypeRepo> &repo)
+             search::IAttributeManager::SP attributeMgr, search::IDocumentStore::SP docStore,
+             std::shared_ptr<const DocumentTypeRepo> repo)
     : _docsumWriter(),
       _wordFolder(std::make_unique<Fast_NormalizeWordFolder>()),
       _juniperProps(juniperCfg),
       _juniperConfig(),
-      _attributeMgr(attributeMgr),
-      _docStore(docStore),
+      _attributeMgr(std::move(attributeMgr)),
+      _docStore(std::move(docStore)),
       _fieldCacheRepo(),
       _repo(repo),
       _markupFields()
@@ -151,7 +150,7 @@ SummaryManager::SummaryManager(vespalib::ThreadExecutor & executor, const LogDoc
                                const search::GrowStrategy & growStrategy, const vespalib::string &baseDir,
                                const DocTypeName &docTypeName, const TuneFileSummary &tuneFileSummary,
                                const FileHeaderContext &fileHeaderContext, search::transactionlog::SyncProxy &tlSyncer,
-                               const search::IBucketizer::SP & bucketizer)
+                               search::IBucketizer::SP bucketizer)
     : _baseDir(baseDir),
       _docTypeName(docTypeName),
       _docStore(),
@@ -159,7 +158,7 @@ SummaryManager::SummaryManager(vespalib::ThreadExecutor & executor, const LogDoc
       _currentSerial(0u)
 {
     _docStore = std::make_shared<LogDocumentStore>(executor, baseDir, storeConfig, growStrategy, tuneFileSummary,
-                                                   fileHeaderContext, tlSyncer, bucketizer);
+                                                   fileHeaderContext, tlSyncer, std::move(bucketizer));
 }
 
 SummaryManager::~SummaryManager() = default;
diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.h b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.h
index 4fde28e69de..f81fdedef83 100644
--- a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.h
+++ b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanager.h
@@ -40,9 +40,9 @@ public:
                      const vespa::config::search::SummaryConfig & summaryCfg,
                      const vespa::config::search::SummarymapConfig & summarymapCfg,
                      const vespa::config::search::summary::JuniperrcConfig & juniperCfg,
-                     const search::IAttributeManager::SP &attributeMgr,
-                     const search::IDocumentStore::SP & docStore,
-                     const std::shared_ptr<const document::DocumentTypeRepo> &repo);
+                     search::IAttributeManager::SP attributeMgr,
+                     search::IDocumentStore::SP docStore,
+                     std::shared_ptr<const document::DocumentTypeRepo> repo);
 
         search::docsummary::IDocsumWriter & getDocsumWriter() const override { return *_docsumWriter; }
         search::docsummary::ResultConfig & getResultConfig() override { return *_docsumWriter->GetResultConfig(); }
@@ -71,8 +71,8 @@ public:
                    const search::TuneFileSummary &tuneFileSummary,
                    const search::common::FileHeaderContext &fileHeaderContext,
                    search::transactionlog::SyncProxy &tlSyncer,
-                   const std::shared_ptr<search::IBucketizer> & bucketizer);
-    ~SummaryManager();
+                   std::shared_ptr<search::IBucketizer> bucketizer);
+    ~SummaryManager() override;
 
     void putDocument(uint64_t syncToken, search::DocumentIdT lid, const document::Document & doc);
     void putDocument(uint64_t syncToken, search::DocumentIdT lid, const vespalib::nbostream & doc);
diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.cpp
index 2f29231577a..29346903aef 100644
--- a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.cpp
+++ b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.cpp
@@ -8,7 +8,7 @@ namespace proton {
 
 SummaryManagerInitializer::
 SummaryManagerInitializer(const search::GrowStrategy &grow,
-                          const vespalib::string baseDir,
+                          const vespalib::string & baseDir,
                           const vespalib::string &subDbName,
                           const DocTypeName &docTypeName,
                           vespalib::ThreadExecutor &summaryExecutor,
diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.h b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.h
index 6c2d3e3d4cc..0d2eb01ee8d 100644
--- a/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.h
+++ b/searchcore/src/vespa/searchcore/proton/docsummary/summarymanagerinitializer.h
@@ -33,7 +33,7 @@ public:
 
     // Note: lifetime of result must be handled by caller.
     SummaryManagerInitializer(const search::GrowStrategy &grow,
-                              const vespalib::string baseDir,
+                              const vespalib::string & baseDir,
                               const vespalib::string &subDbName,
                               const DocTypeName &docTypeName,
                               vespalib::ThreadExecutor & summaryExecutor,
diff --git a/searchlib/src/vespa/searchlib/docstore/compacter.cpp b/searchlib/src/vespa/searchlib/docstore/compacter.cpp
index b53281b4dbb..51c9c4bb5bc 100644
--- a/searchlib/src/vespa/searchlib/docstore/compacter.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/compacter.cpp
@@ -25,11 +25,13 @@ BucketCompacter::BucketCompacter(size_t maxSignificantBucketBits, const Compress
     _ds(ds),
     _bucketizer(bucketizer),
     _writeCount(0),
+    _maxBucketGuardDuration(vespalib::duration::zero()),
+    _lastSample(),
     _lock(),
     _backingMemory(Alloc::alloc(0x40000000), &_lock),
     _tmpStore(),
     _lidGuard(ds.getLidReadGuard()),
-    _bucketizerGuard(bucketizer.getGuard()),
+    _bucketizerGuard(),
     _stat()
 {
     _tmpStore.reserve(256);
@@ -46,13 +48,19 @@ BucketCompacter::getDestinationId(const LockGuard & guard) const {
 void
 BucketCompacter::write(LockGuard guard, uint32_t chunkId, uint32_t lid, const void *buffer, size_t sz)
 {
-    _writeCount++;
+    if (_writeCount++ == 0) {
+        _bucketizerGuard = _bucketizer.getGuard();
+        _lastSample = vespalib::steady_clock::now();
+    }
     guard.unlock();
     BucketId bucketId = (sz > 0) ? _bucketizer.getBucketOf(_bucketizerGuard, lid) : BucketId();
     uint64_t sortableBucketId = bucketId.toKey();
     _tmpStore[(sortableBucketId >> _unSignificantBucketBits) % _tmpStore.size()].add(bucketId, chunkId, lid, buffer, sz);
     if ((_writeCount % 1000) == 0) {
         _bucketizerGuard = _bucketizer.getGuard();
+        vespalib::steady_time now = vespalib::steady_clock::now();
+        _maxBucketGuardDuration = std::max(_maxBucketGuardDuration, now - _lastSample);
+        _lastSample = now;
     }
 }
 
@@ -60,6 +68,7 @@ void
 BucketCompacter::close()
 {
     _bucketizerGuard = GenerationHandler::Guard();
+    vespalib::duration lastBucketGuardDuration = vespalib::steady_clock::now() - _lastSample;
     size_t lidCount1(0);
     size_t bucketCount(0);
     size_t chunkCount(0);
@@ -68,8 +77,9 @@ BucketCompacter::close()
         bucketCount += store.getBucketCount();
         chunkCount += store.getChunkCount();
     }
-    LOG(info, "Have read %ld lids and placed them in %ld buckets. Temporary compressed in %ld chunks.",
-              lidCount1, bucketCount, chunkCount);
+    LOG(info, "Have read %ld lids and placed them in %ld buckets. Temporary compressed in %ld chunks."
+              " Max bucket guard held for %ld us, and last before close for %ld us",
+              lidCount1, bucketCount, chunkCount, vespalib::count_us(_maxBucketGuardDuration), vespalib::count_us(lastBucketGuardDuration));
 
     for (StoreByBucket & store : _tmpStore) {
         store.drain(*this);
diff --git a/searchlib/src/vespa/searchlib/docstore/compacter.h b/searchlib/src/vespa/searchlib/docstore/compacter.h
index 666943ed629..cf059b6cb04 100644
--- a/searchlib/src/vespa/searchlib/docstore/compacter.h
+++ b/searchlib/src/vespa/searchlib/docstore/compacter.h
@@ -49,6 +49,8 @@ private:
     LogDataStore             & _ds;
     const IBucketizer        & _bucketizer;
     uint64_t                   _writeCount;
+    vespalib::duration         _maxBucketGuardDuration;
+    vespalib::steady_time      _lastSample;
     vespalib::Lock             _lock;
     vespalib::MemoryDataStore  _backingMemory;
     std::vector<StoreByBucket> _tmpStore;
diff --git a/searchlib/src/vespa/searchlib/docstore/filechunk.cpp b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
index d21278e2f84..84d64954e40 100644
--- a/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp
@@ -104,7 +104,7 @@ FileChunk::FileChunk(FileId fileId, NameId nameId, const vespalib::string & base
     }
 }
 
-FileChunk::~FileChunk() { }
+FileChunk::~FileChunk() = default;
 
 void
 FileChunk::addNumBuckets(size_t numBucketsInChunk)
@@ -255,20 +255,20 @@ FileChunk::enableRead()
 {
     if (_tune._randRead.getWantDirectIO()) {
         LOG(debug, "enableRead(): DirectIORandRead: file='%s'", _dataFileName.c_str());
-        _file.reset(new DirectIORandRead(_dataFileName));
+        _file = std::make_unique<DirectIORandRead>(_dataFileName);
     } else if (_tune._randRead.getWantMemoryMap()) {
         const int mmapFlags(_tune._randRead.getMemoryMapFlags());
         const int fadviseOptions(_tune._randRead.getAdvise());
         if (frozen()) {
             LOG(debug, "enableRead(): MMapRandRead: file='%s'", _dataFileName.c_str());
-            _file.reset(new MMapRandRead(_dataFileName, mmapFlags, fadviseOptions));
+            _file = std::make_unique<MMapRandRead>(_dataFileName, mmapFlags, fadviseOptions);
         } else {
             LOG(debug, "enableRead(): MMapRandReadDynamic: file='%s'", _dataFileName.c_str());
-            _file.reset(new MMapRandReadDynamic(_dataFileName, mmapFlags, fadviseOptions));
+            _file = std::make_unique<MMapRandReadDynamic>(_dataFileName, mmapFlags, fadviseOptions);
         }
     } else {
         LOG(debug, "enableRead(): NormalRandRead: file='%s'", _dataFileName.c_str());
-        _file.reset(new NormalRandRead(_dataFileName));
+        _file = std::make_unique<NormalRandRead>(_dataFileName);
     }
     _dataHeaderLen = readDataHeader(*_file);
     if (_dataHeaderLen == 0u) {
@@ -326,7 +326,7 @@ appendChunks(FixedParams * args, Chunk::UP chunk)
             }
         }
     }
-    if (args->visitorProgress != NULL) {
+    if (args->visitorProgress != nullptr) {
         args->visitorProgress->updateProgress();
     }
 }
@@ -554,7 +554,6 @@ FileChunk::isIdxFileEmpty(const vespalib::string & name)
     } else {
         throw SummaryException("Failed opening idx file readonly ", idxFile, VESPA_STRLOC);
     }
-    return false;
 }
 
 void
diff --git a/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp b/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp
index 8fd5dc8ec24..91de6ba4276 100644
--- a/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp
@@ -53,7 +53,7 @@ LogDataStore::Config::operator == (const Config & rhs) const {
 LogDataStore::LogDataStore(vespalib::ThreadExecutor &executor, const vespalib::string &dirName, const Config &config,
                            const GrowStrategy &growStrategy, const TuneFileSummary &tune,
                            const FileHeaderContext &fileHeaderContext, transactionlog::SyncProxy &tlSyncer,
-                           const IBucketizer::SP & bucketizer, bool readOnly)
+                           IBucketizer::SP bucketizer, bool readOnly)
     : IDataStore(dirName),
       _config(config),
       _tune(tune),
@@ -70,7 +70,7 @@ LogDataStore::LogDataStore(vespalib::ThreadExecutor &executor, const vespalib::s
       _executor(executor),
       _initFlushSyncToken(0),
       _tlSyncer(tlSyncer),
-      _bucketizer(bucketizer),
+      _bucketizer(std::move(bucketizer)),
       _currentlyCompacting(),
       _compactLidSpaceGeneration()
 {
@@ -182,7 +182,7 @@ LogDataStore::write(uint64_t serialNum, uint32_t lid, const void * buffer, size_
 void
 LogDataStore::write(LockGuard guard, FileId destinationFileId, uint32_t lid, const void * buffer, size_t len)
 {
-    WriteableFileChunk & destination = static_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]);
+    auto & destination = static_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]);
     write(std::move(guard), destination, destination.getSerialNum(), lid, buffer, len);
 }
 
@@ -474,10 +474,10 @@ void LogDataStore::compactFile(FileId fileId)
             setNewFileChunk(guard, createWritableFile(destinationFileId, fc->getLastPersistedSerialNum(), fc->getNameId().next()));
         }
         size_t numSignificantBucketBits = computeNumberOfSignificantBucketIdBits(*_bucketizer, fc->getFileId());
-        compacter.reset(new BucketCompacter(numSignificantBucketBits, _config.compactCompression(), *this, _executor,
-                                            *_bucketizer, fc->getFileId(), destinationFileId));
+        compacter = std::make_unique<BucketCompacter>(numSignificantBucketBits, _config.compactCompression(), *this, _executor,
+                                            *_bucketizer, fc->getFileId(), destinationFileId);
     } else {
-        compacter.reset(new docstore::Compacter(*this));
+        compacter = std::make_unique<docstore::Compacter>(*this);
     }
 
     fc->appendTo(_executor, *this, *compacter, fc->getNumChunks(), nullptr);
@@ -486,7 +486,7 @@ void LogDataStore::compactFile(FileId fileId)
         flushActiveAndWait(0);
     } else {
         LockGuard guard(_updateLock);
-        WriteableFileChunk & compactTo = dynamic_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]);
+        auto & compactTo = dynamic_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]);
         flushFileAndWait(std::move(guard), compactTo, 0);
         compactTo.freeze();
     }
@@ -515,7 +515,7 @@ void LogDataStore::compactFile(FileId fileId)
          * Wait for requireSpace() and flush() methods to leave chunk
          * alone.
          */
-        std::this_thread::sleep_for(1s);;
+        std::this_thread::sleep_for(1s);
     }
     toDie->erase();
     LockGuard guard(_updateLock);
diff --git a/searchlib/src/vespa/searchlib/docstore/logdatastore.h b/searchlib/src/vespa/searchlib/docstore/logdatastore.h
index 6217576172c..39b2c2b7100 100644
--- a/searchlib/src/vespa/searchlib/docstore/logdatastore.h
+++ b/searchlib/src/vespa/searchlib/docstore/logdatastore.h
@@ -87,7 +87,7 @@ public:
     LogDataStore(vespalib::ThreadExecutor &executor, const vespalib::string &dirName, const Config & config,
                  const GrowStrategy &growStrategy, const TuneFileSummary &tune,
                  const search::common::FileHeaderContext &fileHeaderContext,
-                 transactionlog::SyncProxy &tlSyncer, const IBucketizer::SP & bucketizer, bool readOnly = false);
+                 transactionlog::SyncProxy &tlSyncer, IBucketizer::SP bucketizer, bool readOnly = false);
 
     ~LogDataStore() override;
 
diff --git a/searchlib/src/vespa/searchlib/docstore/logdocumentstore.cpp b/searchlib/src/vespa/searchlib/docstore/logdocumentstore.cpp
index c285d4323c2..38be9733cfb 100644
--- a/searchlib/src/vespa/searchlib/docstore/logdocumentstore.cpp
+++ b/searchlib/src/vespa/searchlib/docstore/logdocumentstore.cpp
@@ -20,10 +20,10 @@ LogDocumentStore::LogDocumentStore(vespalib::ThreadExecutor & executor,
                                    const TuneFileSummary & tuneFileSummary,
                                    const FileHeaderContext &fileHeaderContext,
                                    transactionlog::SyncProxy &tlSyncer,
-                                   const IBucketizer::SP & bucketizer)
+                                   IBucketizer::SP bucketizer)
     : DocumentStore(config, _backingStore),
       _backingStore(executor, baseDir, config.getLogConfig(), growStrategy,
-                    tuneFileSummary, fileHeaderContext, tlSyncer, bucketizer)
+                    tuneFileSummary, fileHeaderContext, tlSyncer, std::move(bucketizer))
 {}
 
 LogDocumentStore::~LogDocumentStore() = default;
diff --git a/searchlib/src/vespa/searchlib/docstore/logdocumentstore.h b/searchlib/src/vespa/searchlib/docstore/logdocumentstore.h
index 3c9aefc9934..46e60d9245f 100644
--- a/searchlib/src/vespa/searchlib/docstore/logdocumentstore.h
+++ b/searchlib/src/vespa/searchlib/docstore/logdocumentstore.h
@@ -47,8 +47,8 @@ public:
     LogDocumentStore(vespalib::ThreadExecutor & executor, const vespalib::string & baseDir, const Config & config,
                      const GrowStrategy & growStrategy, const TuneFileSummary &tuneFileSummary,
                      const common::FileHeaderContext &fileHeaderContext,
-                     transactionlog::SyncProxy &tlSyncer, const IBucketizer::SP & bucketizer);
-    ~LogDocumentStore();
+                     transactionlog::SyncProxy &tlSyncer, IBucketizer::SP bucketizer);
+    ~LogDocumentStore() override;
     void reconfigure(const Config & config);
 private:
     void compact(uint64_t syncToken) override       { _backingStore.compact(syncToken); }