From 4bb951d48ec19fe46918e3b9783a389377210561 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Sat, 14 Dec 2019 16:30:04 -0800 Subject: [PATCH] Fix node auto-configuration code: The `node_size` configuration option is used to automatically configure various parameters (cache sizes, timeouts, etc) for the server. A previous commit included changes that caused incorrect values to be returned which can result in sub-optimal performance that can manifest as difficulty syncing to the network, or increased disk I/O and/or memory usage. The problem was introduced with commit 66fad62e661bc8b8739d109e78c18c8d5002c6e9. This commit, if merged, fixes the code to ensure that the correct values are returned and introduces a compile-time check to prevent this issue from reoccurring. --- src/ripple/app/ledger/impl/LedgerMaster.cpp | 2 +- src/ripple/app/main/Application.cpp | 22 ++--- src/ripple/app/misc/SHAMapStoreImp.cpp | 2 +- src/ripple/core/Config.h | 90 +++++++++------------ src/ripple/core/impl/Config.cpp | 68 ++++++++++++++-- src/ripple/nodestore/impl/Shard.cpp | 27 ++++--- 6 files changed, 123 insertions(+), 88 deletions(-) diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index ede4dfb04e..41970f50be 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -152,7 +152,7 @@ LedgerMaster::LedgerMaster (Application& app, Stopwatch& stopwatch, , fetch_depth_ (app_.getSHAMapStore ().clampFetchDepth ( app_.config().FETCH_DEPTH)) , ledger_history_ (app_.config().LEDGER_HISTORY) - , ledger_fetch_size_ (app_.config().getSize (siLedgerFetch)) + , ledger_fetch_size_ (app_.config().getValueFor(SizedItem::ledgerFetch)) , fetch_packs_ ("FetchPack", 65536, std::chrono::seconds {45}, stopwatch, app_.journal("TaggedCache")) { diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index e775b012f2..21fd48bb90 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -866,7 +866,7 @@ public: TxDBInit); mTxnDB->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config_->getSize(siTxnDBCache))); + kilobytes(config_->getValueFor(SizedItem::txnDBCache))); mTxnDB->setupCheckpointing(m_jobQueue.get(), logs()); if (!setup.standAlone || @@ -908,7 +908,7 @@ public: LgrDBInit); mLedgerDB->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config_->getSize(siLgrDBCache))); + kilobytes(config_->getValueFor(SizedItem::lgrDBCache))); mLedgerDB->setupCheckpointing(m_jobQueue.get(), logs()); // wallet database @@ -964,24 +964,24 @@ public: // tune caches using namespace std::chrono; m_nodeStore->tune( - config_->getSize(siNodeCacheSize), - seconds{config_->getSize(siNodeCacheAge)}); + config_->getValueFor(SizedItem::nodeCacheSize), + seconds{config_->getValueFor(SizedItem::nodeCacheAge)}); m_ledgerMaster->tune( - config_->getSize(siLedgerSize), - seconds{config_->getSize(siLedgerAge)}); + config_->getValueFor(SizedItem::ledgerSize), + seconds{config_->getValueFor(SizedItem::ledgerAge)}); family().treecache().setTargetSize( - config_->getSize (siTreeCacheSize)); + config_->getValueFor(SizedItem::treeCacheSize)); family().treecache().setTargetAge( - seconds{config_->getSize(siTreeCacheAge)}); + seconds{config_->getValueFor(SizedItem::treeCacheAge)}); if (sFamily_) { sFamily_->treecache().setTargetSize( - config_->getSize(siTreeCacheSize)); + config_->getValueFor(SizedItem::treeCacheSize)); sFamily_->treecache().setTargetAge( - seconds{config_->getSize(siTreeCacheAge)}); + seconds{config_->getValueFor(SizedItem::treeCacheAge)}); } return true; @@ -1135,7 +1135,7 @@ public: { using namespace std::chrono; sweepTimer_.expires_from_now( - seconds{config_->getSize(siSweepInterval)}); + seconds{config_->getValueFor(SizedItem::sweepInterval)}); sweepTimer_.async_wait (std::move (*optionalCountedHandler)); } } diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index eaa2accf09..6057c239e6 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -190,7 +190,7 @@ SHAMapStoreImp::SHAMapStoreImp( if (!section.exists("cache_mb")) { section.set("cache_mb", std::to_string( - config.getSize(siHashNodeDBCache))); + config.getValueFor(SizedItem::hashNodeDBCache))); } if (!section.exists("filter_bits") && (config.NODE_SIZE >= 2)) diff --git a/src/ripple/core/Config.h b/src/ripple/core/Config.h index fca0b0f5aa..d77241b2eb 100644 --- a/src/ripple/core/Config.h +++ b/src/ripple/core/Config.h @@ -29,6 +29,7 @@ #include // VFALCO FIX: This include should not be here #include #include +#include #include #include #include @@ -42,47 +43,21 @@ class Rules; //------------------------------------------------------------------------------ -enum SizedItemName +enum class SizedItem : std::size_t { - siSweepInterval = 0, - siNodeCacheSize, - siNodeCacheAge, - siTreeCacheSize, - siTreeCacheAge, - siSLECacheSize, - siSLECacheAge, - siLedgerSize, - siLedgerAge, - siLedgerFetch, - siHashNodeDBCache, - siTxnDBCache, - siLgrDBCache + sweepInterval = 0, + treeCacheSize, + treeCacheAge, + ledgerSize, + ledgerAge, + ledgerFetch, + nodeCacheSize, + nodeCacheAge, + hashNodeDBCache, + txnDBCache, + lgrDBCache }; -static constexpr -std::array, 13> sizedItems -{{ - // tiny small medium large huge - {{ 10, 30, 60, 90, 120 }}, // siSweepInterval - {{ 2, 3, 5, 5, 8 }}, // siLedgerFetch - - {{ 16384, 32768, 131072, 262144, 524288 }}, // siNodeCacheSize - {{ 60, 90, 120, 900, 1800 }}, // siNodeCacheAge - - {{ 128000, 256000, 512000, 768000, 2048000 }}, // siTreeCacheSize - {{ 30, 60, 90, 120, 900 }}, // siTreeCacheAge - - {{ 4096, 8192, 16384, 65536, 131072 }}, // siSLECacheSize - {{ 30, 60, 90, 120, 300 }}, // siSLECacheAge - - {{ 32, 128, 256, 384, 768 }}, // siLedgerSize - {{ 30, 90, 180, 240, 900 }}, // siLedgerAge - - {{ 4, 12, 24, 64, 128 }}, // siHashNodeDBCache - {{ 4, 12, 24, 64, 128 }}, // siTxnDBCache - {{ 4, 8, 16, 32, 128 }} // siLgrDBCache -}}; - // This entire derived class is deprecated. // For new config information use the style implied // in the base class. For existing config information @@ -185,7 +160,8 @@ public: // Node storage configuration std::uint32_t LEDGER_HISTORY = 256; std::uint32_t FETCH_DEPTH = 1000000000; - int NODE_SIZE = 0; + + std::size_t NODE_SIZE = 0; bool SSL_VERIFY = true; std::string SSL_VERIFY_FILE; @@ -202,21 +178,6 @@ public: public: Config() : j_ {beast::Journal::getNullSink()} {} - static - int - getSize(SizedItemName item, std::uint32_t nodeSize) - { - assert(item < sizedItems.size() && nodeSize < sizedItems[item].size()); - return sizedItems[item][nodeSize]; - } - - int - getSize(SizedItemName item) const - { - assert(item < sizedItems.size()); - return getSize(item, NODE_SIZE); - } - /* Be very careful to make sure these bool params are in the right order. */ void setup (std::string const& strConf, bool bQuiet, @@ -236,6 +197,27 @@ public: bool standalone() const { return RUN_STANDALONE; } bool canSign() const { return signingEnabled_; } + + /** Retrieve the default value for the item at the specified node size + + @param item The item for which the default value is needed + @param node Optional value, used to adjust the result to match the + size of a node (0: tiny, ..., 4: huge). If unseated, + uses the configured size (NODE_SIZE). + + @throw This method can throw std::out_of_range if you ask for values + that it does not recognize or request a non-default node-size. + + @return The value for the requested item. + + @note The defaults are selected so as to be reasonable, but the node + size is an imprecise metric that combines multiple aspects of + the underlying system; this means that we can't provide optimal + defaults in the code for every case. + */ + int + getValueFor(SizedItem item, + boost::optional node = boost::none) const; }; } // ripple diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 5464799cc0..0d256f635c 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -32,12 +32,61 @@ #include #include #include +#include #include #include #include namespace ripple { +inline constexpr +std::array>, 11> +sizedItems +{{ + // FIXME: We should document each of these items, explaining exactly what + // they control and whether there exists an explicit config option + // that can be used to override the default. + { SizedItem::sweepInterval, + {{ 10, 30, 60, 90, 120 }} }, + { SizedItem::treeCacheSize, + {{ 128000, 256000, 512000, 768000, 2048000 }} }, + { SizedItem::treeCacheAge, + {{ 30, 60, 90, 120, 900 }} }, + { SizedItem::ledgerSize, + {{ 32, 128, 256, 384, 768 }} }, + { SizedItem::ledgerAge, + {{ 30, 90, 180, 240, 900 }} }, + { SizedItem::ledgerFetch, + {{ 2, 3, 4, 5, 8 }} }, + { SizedItem::nodeCacheSize, + {{ 16384, 32768, 131072, 262144, 524288 }} }, + { SizedItem::nodeCacheAge, + {{ 60, 90, 120, 900, 1800 }} }, + { SizedItem::hashNodeDBCache, + {{ 4, 12, 24, 64, 128 }} }, + { SizedItem::txnDBCache, + {{ 4, 12, 24, 64, 128 }} }, + { SizedItem::lgrDBCache, + {{ 4, 8, 16, 32, 128 }} }, +}}; + +// Ensure that the order of entries in the table corresponds to the +// order of entries in the enum: +static_assert([]() constexpr -> bool +{ + std::underlying_type_t idx = 0; + + for (auto const& i : sizedItems) + { + if (static_cast>(i.first) != idx) + return false; + + ++idx; + } + + return true; +}(), "Mismatch between sized item enum & array indices"); + // // TODO: Check permissions on config file before using it. // @@ -329,14 +378,8 @@ void Config::loadFromString (std::string const& fileContents) else if (boost::iequals(strTemp, "huge")) NODE_SIZE = 4; else - { - NODE_SIZE = beast::lexicalCastThrow (strTemp); - - if (NODE_SIZE < 0) - NODE_SIZE = 0; - else if (NODE_SIZE > 4) - NODE_SIZE = 4; - } + NODE_SIZE = std::min(4, + beast::lexicalCastThrow(strTemp)); } if (getSingleSection (secConfig, SECTION_SIGNING_SUPPORT, strTemp, j_)) @@ -594,4 +637,13 @@ boost::filesystem::path Config::getDebugLogFile () const return log_file; } +int +Config::getValueFor(SizedItem item, boost::optional node) const +{ + auto const index = static_cast>(item); + assert(index < sizedItems.size()); + assert(!node || *node <= 4); + return sizedItems.at(index).second.at(node.value_or(NODE_SIZE)); +} + } // ripple diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 2b7b5cddf2..2b685a661c 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -422,24 +422,25 @@ Shard::setCache(std::lock_guard const&) if (!pCache_) { auto const name {"shard " + std::to_string(index_)}; - auto const sz {complete_ ? - Config::getSize(siNodeCacheSize, 0) : - app_.config().getSize(siNodeCacheSize)}; - auto const age {std::chrono::seconds{complete_ ? - Config::getSize(siNodeCacheAge, 0) : - app_.config().getSize(siNodeCacheAge)}}; + auto const sz = app_.config().getValueFor(SizedItem::nodeCacheSize, + complete_ ? boost::optional(0) : boost::none); + auto const age = std::chrono::seconds{ + app_.config().getValueFor(SizedItem::nodeCacheAge, + complete_ ? boost::optional(0) : boost::none)}; pCache_ = std::make_shared(name, sz, age, stopwatch(), j_); nCache_ = std::make_shared(name, stopwatch(), sz, age); } else { - auto const sz {Config::getSize(siNodeCacheSize, 0)}; + auto const sz = app_.config().getValueFor( + SizedItem::nodeCacheSize, 0); pCache_->setTargetSize(sz); nCache_->setTargetSize(sz); - auto const age {std::chrono::seconds{ - Config::getSize(siNodeCacheAge, 0)}}; + auto const age = std::chrono::seconds{ + app_.config().getValueFor( + SizedItem::nodeCacheAge, 0)}; pCache_->setTargetAge(age); nCache_->setTargetAge(age); } @@ -496,7 +497,7 @@ Shard::initSQLite(std::lock_guard const&) LgrDBInit); lgrSQLiteDB_->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(Config::getSize(siLgrDBCache, 0))); + kilobytes(config.getValueFor(SizedItem::lgrDBCache, boost::none))); txSQLiteDB_ = std::make_unique ( setup, @@ -505,7 +506,7 @@ Shard::initSQLite(std::lock_guard const&) TxDBInit); txSQLiteDB_->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(Config::getSize(siTxnDBCache, 0))); + kilobytes(config.getValueFor(SizedItem::txnDBCache, boost::none))); } else { @@ -517,7 +518,7 @@ Shard::initSQLite(std::lock_guard const&) LgrDBInit); lgrSQLiteDB_->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config.getSize(siLgrDBCache))); + kilobytes(config.getValueFor(SizedItem::lgrDBCache))); lgrSQLiteDB_->setupCheckpointing(&app_.getJobQueue(), app_.logs()); txSQLiteDB_ = std::make_unique ( @@ -527,7 +528,7 @@ Shard::initSQLite(std::lock_guard const&) TxDBInit); txSQLiteDB_->getSession() << boost::str(boost::format("PRAGMA cache_size=-%d;") % - kilobytes(config.getSize(siTxnDBCache))); + kilobytes(config.getValueFor(SizedItem::txnDBCache))); txSQLiteDB_->setupCheckpointing(&app_.getJobQueue(), app_.logs()); } }