[FOLD] Address feedback

src/ripple/app/ledger/AccountStateSF.cpp

@@ -23,15 +23,16 @@
 namespace ripple {
 
 void
-AccountStateSF::gotNode(bool fromFilter, SHAMapHash const& nodeHash,
- std::uint32_t ledgerSeq, Blob&& nodeData, SHAMapTreeNode::TNType type) const
+AccountStateSF::gotNode(bool, SHAMapHash const& nodeHash,
+ std::uint32_t ledgerSeq, Blob&& nodeData,
+ SHAMapTreeNode::TNType) const
 {
  db_.store(hotACCOUNT_NODE, std::move(nodeData),
  nodeHash.as_uint256(), ledgerSeq);
 }
 
 boost::optional<Blob>
-AccountStateSF::getNode(SHAMapHash const& nodeHash, std::uint32_t) const
+AccountStateSF::getNode(SHAMapHash const& nodeHash) const
 {
  return fp_.getFetchPack(nodeHash.as_uint256());
 }

src/ripple/app/ledger/AccountStateSF.h

@@ -42,8 +42,7 @@ class AccountStateSF : public SHAMapSyncFilter
  SHAMapTreeNode::TNType type) const override;
 
  boost::optional<Blob>
- getNode(SHAMapHash const& nodeHash,
- std::uint32_t ledgerSeq) const override;
+ getNode(SHAMapHash const& nodeHash) const override;
 
 private:
  NodeStore::Database& db_;

src/ripple/app/ledger/ConsensusTransSetSF.cpp

@@ -76,7 +76,7 @@ ConsensusTransSetSF::gotNode(bool fromFilter, SHAMapHash const& nodeHash,
 }
 
 boost::optional<Blob>
-ConsensusTransSetSF::getNode (SHAMapHash const& nodeHash, std::uint32_t) const
+ConsensusTransSetSF::getNode (SHAMapHash const& nodeHash) const
 {
  Blob nodeData;
  if (m_nodeCache.retrieve (nodeHash, nodeData))

src/ripple/app/ledger/ConsensusTransSetSF.h

@@ -45,8 +45,7 @@ class ConsensusTransSetSF : public SHAMapSyncFilter
  SHAMapTreeNode::TNType type) const override;
 
  boost::optional<Blob>
- getNode (SHAMapHash const& nodeHash,
- std::uint32_t ledgerSeq) const override;
+ getNode (SHAMapHash const& nodeHash) const override;
 
 private:
  Application& app_;

src/ripple/app/ledger/TransactionStateSF.cpp

@@ -23,7 +23,7 @@
 namespace ripple {
 
 void
-TransactionStateSF::gotNode(bool fromFilter, SHAMapHash const& nodeHash,
+TransactionStateSF::gotNode(bool, SHAMapHash const& nodeHash,
  std::uint32_t ledgerSeq, Blob&& nodeData, SHAMapTreeNode::TNType type) const
 
 {
@@ -33,8 +33,7 @@ TransactionStateSF::gotNode(bool fromFilter, SHAMapHash const& nodeHash,
 }
 
 boost::optional<Blob>
-TransactionStateSF::getNode(SHAMapHash const& nodeHash,
- std::uint32_t ledgerSeq) const
+TransactionStateSF::getNode(SHAMapHash const& nodeHash) const
 {
  return fp_.getFetchPack(nodeHash.as_uint256());
 }

src/ripple/app/ledger/TransactionStateSF.h

@@ -42,8 +42,7 @@ class TransactionStateSF : public SHAMapSyncFilter
  SHAMapTreeNode::TNType type) const override;
 
  boost::optional<Blob>
- getNode(SHAMapHash const& nodeHash,
- std::uint32_t ledgerSeq) const override;
+ getNode(SHAMapHash const& nodeHash) const override;
 
 private:
  NodeStore::Database& db_;

src/ripple/basics/TaggedCache.h

@@ -478,7 +478,7 @@ class TaggedCache
  return m_mutex;
  }
 
- std::vector <key_type> getKeys ()
+ std::vector <key_type> getKeys () const
  {
  std::vector <key_type> v;
 

src/ripple/nodestore/Database.h

@@ -226,8 +226,8 @@ class Database : public Stoppable
 
  void
  asyncFetch(uint256 const& hash, std::uint32_t seq,
- TaggedCache<uint256, NodeObject>& pCache,
- KeyCache<uint256>& nCache);
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> const& pCache,
+ std::shared_ptr<KeyCache<uint256>> const& nCache);
 
  std::shared_ptr<NodeObject>
  fetchInternal(uint256 const& hash, Backend& backend);
@@ -237,8 +237,8 @@ class Database : public Stoppable
 
  std::shared_ptr<NodeObject>
  doFetch(uint256 const& hash, std::uint32_t seq,
- TaggedCache<uint256, NodeObject>& pCache,
- KeyCache<uint256>& nCache, bool isAsync);
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> const& pCache,
+ std::shared_ptr<KeyCache<uint256>> const& nCache, bool isAsync);
 
 private:
  std::atomic<std::uint32_t> storeCount_ {0};
@@ -250,13 +250,20 @@ class Database : public Stoppable
  std::mutex readLock_;
  std::condition_variable readCondVar_;
  std::condition_variable readGenCondVar_;
+
+ // reads to do
  std::map<uint256, std::tuple<std::uint32_t,
- TaggedCache<uint256, NodeObject>*,
- KeyCache<uint256>*>> read_; // reads to do
- uint256 readLastHash_; // last read
+ std::weak_ptr<TaggedCache<uint256, NodeObject>>,
+ std::weak_ptr<KeyCache<uint256>>>> read_;
+
+ // last read
+ uint256 readLastHash_;
+
  std::vector<std::thread> readThreads_;
  bool readShut_ {false};
- uint64_t readGen_ {0}; // current read generation
+
+ // current read generation
+ uint64_t readGen_ {0};
 
  virtual
  std::shared_ptr<NodeObject>

src/ripple/nodestore/DatabaseRotating.h

@@ -38,7 +38,9 @@ class DatabaseRotating : public Database
  : Database(name, parent, scheduler, readThreads, journal)
  {}
 
- virtual TaggedCache <uint256, NodeObject>& getPositiveCache() = 0;
+ virtual
+ TaggedCache<uint256, NodeObject> const&
+ getPositiveCache() = 0;
 
  virtual std::mutex& peekMutex() const = 0;
 

src/ripple/nodestore/impl/Database.cpp

@@ -84,12 +84,12 @@ Database::stopThreads()
 
 void
 Database::asyncFetch(uint256 const& hash, std::uint32_t seq,
- TaggedCache<uint256, NodeObject>& pCache,
- KeyCache<uint256>& nCache)
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> const& pCache,
+ std::shared_ptr<KeyCache<uint256>> const& nCache)
 {
  // Post a read
  std::lock_guard <std::mutex> l(readLock_);
- if (read_.emplace(hash, std::make_tuple(seq, &pCache, &nCache)).second)
+ if (read_.emplace(hash, std::make_tuple(seq, pCache, nCache)).second)
  readCondVar_.notify_one();
 }
 
@@ -161,8 +161,8 @@ Database::importInternal(Database& source, Backend& dest)
 // Perform a fetch and report the time it took
 std::shared_ptr<NodeObject>
 Database::doFetch(uint256 const& hash, std::uint32_t seq,
- TaggedCache<uint256, NodeObject>& pCache,
- KeyCache<uint256>& nCache, bool isAsync)
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> const& pCache,
+ std::shared_ptr<KeyCache<uint256>> const& nCache, bool isAsync)
 {
  FetchReport report;
  report.isAsync = isAsync;
@@ -172,8 +172,8 @@ Database::doFetch(uint256 const& hash, std::uint32_t seq,
  auto const before = steady_clock::now();
 
  // See if the object already exists in the cache
- auto nObj = pCache.fetch(hash);
- if (! nObj && ! nCache.touch_if_exists(hash))
+ auto nObj = pCache->fetch(hash);
+ if (! nObj && ! nCache->touch_if_exists(hash))
  {
  // Try the database(s)
  report.wentToDisk = true;
@@ -182,15 +182,15 @@ Database::doFetch(uint256 const& hash, std::uint32_t seq,
  if (! nObj)
  {
  // Just in case a write occurred
- nObj = pCache.fetch(hash);
+ nObj = pCache->fetch(hash);
  if (! nObj)
  // We give up
- nCache .insert(hash);
+ nCache->insert(hash);
  }
  else
  {
  // Ensure all threads get the same object
- pCache.canonicalize(hash, nObj);
+ pCache->canonicalize(hash, nObj);
 
  // Since this was a 'hard' fetch, we will log it.
  JLOG(j_.trace()) <<
@@ -213,8 +213,8 @@ Database::threadEntry()
  {
  uint256 lastHash;
  std::uint32_t lastSeq;
- TaggedCache<uint256, NodeObject>* lastPcache;
- KeyCache<uint256>* lastNcache;
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> lastPcache;
+ std::shared_ptr<KeyCache<uint256>> lastNcache;
  {
  std::unique_lock<std::mutex> l(readLock_);
  while (! readShut_ && read_.empty())
@@ -237,14 +237,15 @@ Database::threadEntry()
  }
  lastHash = it->first;
  lastSeq = std::get<0>(it->second);
- lastPcache = std::get<1>(it->second);
- lastNcache = std::get<2>(it->second);
+ lastPcache = std::get<1>(it->second).lock();
+ lastNcache = std::get<2>(it->second).lock();
  read_.erase(it);
  readLastHash_ = lastHash;
  }
 
  // Perform the read
- doFetch(lastHash, lastSeq, *lastPcache, *lastNcache, true);
+ if (lastPcache && lastPcache)
+ doFetch(lastHash, lastSeq, lastPcache, lastNcache, true);
  }
 }
 

src/ripple/nodestore/impl/DatabaseNodeImp.cpp

@@ -33,9 +33,9 @@ DatabaseNodeImp::store(NodeObjectType type, Blob&& data,
  assert(hash == sha512Hash(makeSlice(data)));
 #endif
  auto nObj = NodeObject::createObject(type, std::move(data), hash);
- pCache_.canonicalize(hash, nObj, true);
+ pCache_->canonicalize(hash, nObj, true);
  backend_->store(nObj);
- nCache_.erase(hash);
+ nCache_->erase(hash);
  storeStats(nObj->getData().size());
 }
 
@@ -44,8 +44,8 @@ DatabaseNodeImp::asyncFetch(uint256 const& hash,
  std::uint32_t seq, std::shared_ptr<NodeObject>& object)
 {
  // See if the object is in cache
- object = pCache_.fetch(hash);
- if (object || nCache_.touch_if_exists(hash))
+ object = pCache_->fetch(hash);
+ if (object || nCache_->touch_if_exists(hash))
  return true;
  // Otherwise post a read
  Database::asyncFetch(hash, seq, pCache_, nCache_);
@@ -123,8 +123,8 @@ DatabaseNodeImp::copyLedger(
 #if RIPPLE_VERIFY_NODEOBJECT_KEYS
  assert(nObj->getHash() == sha512Hash(makeSlice(nObj->getData())));
 #endif
- pCache_.canonicalize(nObj->getHash(), nObj, true);
- nCache_.erase(nObj->getHash());
+ pCache_->canonicalize(nObj->getHash(), nObj, true);
+ nCache_->erase(nObj->getHash());
  storeStats(nObj->getData().size());
  }
  backend_->storeBatch(batch);
@@ -134,17 +134,17 @@ DatabaseNodeImp::copyLedger(
 void
 DatabaseNodeImp::tune(int size, int age)
 {
- pCache_.setTargetSize(size);
- pCache_.setTargetAge(age);
- nCache_.setTargetSize(size);
- nCache_.setTargetAge(age);
+ pCache_->setTargetSize(size);
+ pCache_->setTargetAge(age);
+ nCache_->setTargetSize(size);
+ nCache_->setTargetAge(age);
 }
 
 void
 DatabaseNodeImp::sweep()
 {
- pCache_.sweep();
- nCache_.sweep();
+ pCache_->sweep();
+ nCache_->sweep();
 }
 
 } // NodeStore

src/ripple/nodestore/impl/DatabaseNodeImp.h

@@ -37,8 +37,10 @@ class DatabaseNodeImp : public Database
  Scheduler& scheduler, int readThreads, Stoppable& parent,
  std::unique_ptr<Backend> backend, beast::Journal j)
  : Database(name, parent, scheduler, readThreads, j)
- , pCache_(name, cacheTargetSize, cacheTargetSeconds, stopwatch(), j)
- , nCache_(name, stopwatch(), cacheTargetSize, cacheTargetSeconds)
+ , pCache_(std::make_shared<TaggedCache<uint256, NodeObject>>(
+ name, cacheTargetSize, cacheTargetSeconds, stopwatch(), j))
+ , nCache_(std::make_shared<KeyCache<uint256>>(
+ name, stopwatch(), cacheTargetSize, cacheTargetSeconds))
  , backend_(std::move(backend))
  {
  assert(backend_);
@@ -91,11 +93,11 @@ class DatabaseNodeImp : public Database
  // We prefer a client not fill our cache
  // We don't want to push data out of the cache
  // before it's retrieved
- return pCache_.getTargetSize() / asyncDivider;
+ return pCache_->getTargetSize() / asyncDivider;
  }
 
  float
- getCacheHitRate() override { return pCache_.getHitRate(); }
+ getCacheHitRate() override {return pCache_->getHitRate();}
 
  void
  tune(int size, int age) override;
@@ -105,10 +107,10 @@ class DatabaseNodeImp : public Database
 
 private:
  // Positive cache
- TaggedCache<uint256, NodeObject> pCache_;
+ std::shared_ptr<TaggedCache<uint256, NodeObject>> pCache_;
 
  // Negative cache
- KeyCache<uint256> nCache_;
+ std::shared_ptr<KeyCache<uint256>> nCache_;
 
  // Persistent key/value storage
  std::unique_ptr<Backend> backend_;