Simplify and improve order book tracking:

- Avoid using std::shared_ptr
- Prefer using unordered maps to avoid linear searches
- Increase the interval between full order book updates

src/ripple/app/ledger/OrderBookDB.cpp

@@ -20,6 +20,7 @@
 #include <ripple/app/ledger/LedgerMaster.h>
 #include <ripple/app/ledger/OrderBookDB.h>
 #include <ripple/app/main/Application.h>
+#include <ripple/app/misc/NetworkOPs.h>
 #include <ripple/basics/Log.h>
 #include <ripple/core/Config.h>
 #include <ripple/core/JobQueue.h>
@@ -28,70 +29,72 @@
 namespace ripple {
 
 OrderBookDB::OrderBookDB(Application& app)
-    : app_(app), mSeq(0), j_(app.journal("OrderBookDB"))
+    : app_(app), seq_(0), j_(app.journal("OrderBookDB"))
 {
 }
 
-void
-OrderBookDB::invalidate()
-{
-    std::lock_guard sl(mLock);
-    mSeq = 0;
-}
-
 void
 OrderBookDB::setup(std::shared_ptr<ReadView const> const& ledger)
 {
+    if (!app_.config().standalone() && app_.getOPs().isNeedNetworkLedger())
     {
-        std::lock_guard sl(mLock);
+        JLOG(j_.warn()) << "Eliding full order book update: no ledger";
-        auto seq = ledger->info().seq;
+        return;
-
-        // Do a full update every 256 ledgers
-        if (mSeq != 0)
-        {
-            if (seq == mSeq)
-                return;
-            if ((seq > mSeq) && ((seq - mSeq) < 256))
-                return;
-            if ((seq < mSeq) && ((mSeq - seq) < 16))
-                return;
-        }
-
-        JLOG(j_.debug()) << "Advancing from " << mSeq << " to " << seq;
-
-        mSeq = seq;
     }
 
+    auto seq = seq_.load();
+
+    if (seq != 0)
+    {
+        if ((seq > ledger->seq()) && ((ledger->seq() - seq) < 25600))
+            return;
+
+        if ((ledger->seq() <= seq) && ((seq - ledger->seq()) < 16))
+            return;
+    }
+
+    if (seq_.exchange(ledger->seq()) != seq)
+        return;
+
+    JLOG(j_.debug()) << "Full order book update: " << seq << " to "
+                     << ledger->seq();
+
     if (app_.config().PATH_SEARCH_MAX != 0)
     {
         if (app_.config().standalone())
             update(ledger);
         else
             app_.getJobQueue().addJob(
-                jtUPDATE_PF, "OrderBookDB::update", [this, ledger]() {
+                jtUPDATE_PF,
-                    update(ledger);
+                "OrderBookDB::update: " + std::to_string(ledger->seq()),
-                });
+                [this, ledger]() { update(ledger); });
     }
 }
 
 void
 OrderBookDB::update(std::shared_ptr<ReadView const> const& ledger)
 {
-    hash_set<uint256> seen;
-    OrderBookDB::IssueToOrderBook destMap;
-    OrderBookDB::IssueToOrderBook sourceMap;
-    hash_set<Issue> XRPBooks;
-
-    JLOG(j_.debug()) << "OrderBookDB::update>";
-
     if (app_.config().PATH_SEARCH_MAX == 0)
+        return;  // pathfinding has been disabled
+
+    // A newer full update job is pending
+    if (auto const seq = seq_.load(); seq > ledger->seq())
     {
-        // pathfinding has been disabled
+        JLOG(j_.debug()) << "Eliding update for " << ledger->seq()
+                         << " because of pending update to later " << seq;
         return;
     }
 
+    decltype(allBooks_) allBooks;
+    decltype(xrpBooks_) xrpBooks;
+
+    allBooks.reserve(allBooks_.size());
+    xrpBooks.reserve(xrpBooks_.size());
+
+    JLOG(j_.debug()) << "Beginning update (" << ledger->seq() << ")";
+
     // walk through the entire ledger looking for orderbook entries
-    int books = 0;
+    int cnt = 0;
 
     try
     {
@@ -100,9 +103,8 @@ OrderBookDB::update(std::shared_ptr<ReadView const> const& ledger)
             if (app_.isStopping())
             {
                 JLOG(j_.info())
-                    << "OrderBookDB::update exiting due to isStopping";
+                    << "Update halted because the process is stopping";
-                std::lock_guard sl(mLock);
+                seq_.store(0);
-                mSeq = 0;
                 return;
             }
 
@@ -111,40 +113,38 @@ OrderBookDB::update(std::shared_ptr<ReadView const> const& ledger)
                 sle->getFieldH256(sfRootIndex) == sle->key())
             {
                 Book book;
+
                 book.in.currency = sle->getFieldH160(sfTakerPaysCurrency);
                 book.in.account = sle->getFieldH160(sfTakerPaysIssuer);
-                book.out.account = sle->getFieldH160(sfTakerGetsIssuer);
                 book.out.currency = sle->getFieldH160(sfTakerGetsCurrency);
+                book.out.account = sle->getFieldH160(sfTakerGetsIssuer);
 
-                uint256 index = getBookBase(book);
+                allBooks[book.in].insert(book.out);
-                if (seen.insert(index).second)
+
-                {
+                if (isXRP(book.out))
-                    auto orderBook = std::make_shared<OrderBook>(index, book);
+                    xrpBooks.insert(book.in);
-                    sourceMap[book.in].push_back(orderBook);
+
-                    destMap[book.out].push_back(orderBook);
+                ++cnt;
-                    if (isXRP(book.out))
-                        XRPBooks.insert(book.in);
-                    ++books;
-                }
             }
         }
     }
     catch (SHAMapMissingNode const& mn)
     {
-        JLOG(j_.info()) << "OrderBookDB::update: " << mn.what();
+        JLOG(j_.info()) << "Missing node in " << ledger->seq()
-        std::lock_guard sl(mLock);
+                        << " during update: " << mn.what();
-        mSeq = 0;
+        seq_.store(0);
         return;
     }
 
-    JLOG(j_.debug()) << "OrderBookDB::update< " << books << " books found";
+    JLOG(j_.debug()) << "Update completed (" << ledger->seq() << "): " << cnt
+                     << " books found";
+
     {
         std::lock_guard sl(mLock);
-
+        allBooks_.swap(allBooks);
-        mXRPBooks.swap(XRPBooks);
+        xrpBooks_.swap(xrpBooks);
-        mSourceMap.swap(sourceMap);
-        mDestMap.swap(destMap);
     }
+
     app_.getLedgerMaster().newOrderBookDB();
 }
 
@@ -152,60 +152,50 @@ void
 OrderBookDB::addOrderBook(Book const& book)
 {
     bool toXRP = isXRP(book.out);
+
     std::lock_guard sl(mLock);
 
-    if (toXRP)
+    allBooks_[book.in].insert(book.out);
-    {
-        // We don't want to search through all the to-XRP or from-XRP order
-        // books!
-        for (auto ob : mSourceMap[book.in])
-        {
-            if (isXRP(ob->getCurrencyOut()))  // also to XRP
-                return;
-        }
-    }
-    else
-    {
-        for (auto ob : mDestMap[book.out])
-        {
-            if (ob->getCurrencyIn() == book.in.currency &&
-                ob->getIssuerIn() == book.in.account)
-            {
-                return;
-            }
-        }
-    }
-    uint256 index = getBookBase(book);
-    auto orderBook = std::make_shared<OrderBook>(index, book);
 
-    mSourceMap[book.in].push_back(orderBook);
-    mDestMap[book.out].push_back(orderBook);
     if (toXRP)
-        mXRPBooks.insert(book.in);
+        xrpBooks_.insert(book.in);
 }
 
 // return list of all orderbooks that want this issuerID and currencyID
-OrderBook::List
+std::vector<Book>
 OrderBookDB::getBooksByTakerPays(Issue const& issue)
 {
-    std::lock_guard sl(mLock);
+    std::vector<Book> ret;
-    auto it = mSourceMap.find(issue);
+
-    return it == mSourceMap.end() ? OrderBook::List() : it->second;
+    {
+        std::lock_guard sl(mLock);
+
+        if (auto it = allBooks_.find(issue); it != allBooks_.end())
+        {
+            ret.reserve(it->second.size());
+
+            for (auto const& gets : it->second)
+                ret.push_back(Book(issue, gets));
+        }
+    }
+
+    return ret;
 }
 
 int
 OrderBookDB::getBookSize(Issue const& issue)
 {
     std::lock_guard sl(mLock);
-    auto it = mSourceMap.find(issue);
+    if (auto it = allBooks_.find(issue); it != allBooks_.end())
-    return it == mSourceMap.end() ? 0 : it->second.size();
+        return static_cast<int>(it->second.size());
+    return 0;
 }
 
 bool
 OrderBookDB::isBookToXRP(Issue const& issue)
 {
     std::lock_guard sl(mLock);
-    return mXRPBooks.count(issue) > 0;
+    return xrpBooks_.count(issue) > 0;
 }
 
 BookListeners::pointer
@@ -247,63 +237,52 @@ OrderBookDB::processTxn(
     Json::Value const& jvObj)
 {
     std::lock_guard sl(mLock);
-    if (alTx.getResult() == tesSUCCESS)
+
+    // For this particular transaction, maintain the set of unique
+    // subscriptions that have already published it.  This prevents sending
+    // the transaction multiple times if it touches multiple ltOFFER
+    // entries for the same book, or if it touches multiple books and a
+    // single client has subscribed to those books.
+    hash_set<std::uint64_t> havePublished;
+
+    // Check if this is an offer or an offer cancel or a payment that
+    // consumes an offer.
+    // Check to see what the meta looks like.
+    for (auto& node : alTx.getMeta()->getNodes())
     {
-        // For this particular transaction, maintain the set of unique
+        try
-        // subscriptions that have already published it.  This prevents sending
-        // the transaction multiple times if it touches multiple ltOFFER
-        // entries for the same book, or if it touches multiple books and a
-        // single client has subscribed to those books.
-        hash_set<std::uint64_t> havePublished;
-
-        // Check if this is an offer or an offer cancel or a payment that
-        // consumes an offer.
-        // Check to see what the meta looks like.
-        for (auto& node : alTx.getMeta()->getNodes())
         {
-            try
+            if (node.getFieldU16(sfLedgerEntryType) == ltOFFER)
             {
-                if (node.getFieldU16(sfLedgerEntryType) == ltOFFER)
+                auto process = [&, this](SField const& field) {
-                {
+                    if (auto data = dynamic_cast<STObject const*>(
-                    SField const* field = nullptr;
+                            node.peekAtPField(field));
-
+                        data && data->isFieldPresent(sfTakerPays) &&
-                    // We need a field that contains the TakerGets and TakerPays
+                        data->isFieldPresent(sfTakerGets))
-                    // parameters.
-                    if (node.getFName() == sfModifiedNode)
-                        field = &sfPreviousFields;
-                    else if (node.getFName() == sfCreatedNode)
-                        field = &sfNewFields;
-                    else if (node.getFName() == sfDeletedNode)
-                        field = &sfFinalFields;
-
-                    if (field)
                     {
-                        auto data = dynamic_cast<const STObject*>(
+                        auto listeners = getBookListeners(
-                            node.peekAtPField(*field));
+                            {data->getFieldAmount(sfTakerGets).issue(),
-
+                             data->getFieldAmount(sfTakerPays).issue()});
-                        if (data && data->isFieldPresent(sfTakerPays) &&
+                        if (listeners)
-                            data->isFieldPresent(sfTakerGets))
+                            listeners->publish(jvObj, havePublished);
-                        {
-                            // determine the OrderBook
-                            Book b{
-                                data->getFieldAmount(sfTakerGets).issue(),
-                                data->getFieldAmount(sfTakerPays).issue()};
-
-                            auto listeners = getBookListeners(b);
-                            if (listeners)
-                            {
-                                listeners->publish(jvObj, havePublished);
-                            }
-                        }
                     }
-                }
+                };
-            }
+
-            catch (std::exception const&)
+                // We need a field that contains the TakerGets and TakerPays
-            {
+                // parameters.
-                JLOG(j_.info())
+                if (node.getFName() == sfModifiedNode)
-                    << "Fields not found in OrderBookDB::processTxn";
+                    process(sfPreviousFields);
+                else if (node.getFName() == sfCreatedNode)
+                    process(sfNewFields);
+                else if (node.getFName() == sfDeletedNode)
+                    process(sfFinalFields);
             }
         }
+        catch (std::exception const& ex)
+        {
+            JLOG(j_.info())
+                << "processTxn: field not found (" << ex.what() << ")";
+        }
     }
 }
 

src/ripple/app/ledger/OrderBookDB.h

@@ -23,7 +23,6 @@
 #include <ripple/app/ledger/AcceptedLedgerTx.h>
 #include <ripple/app/ledger/BookListeners.h>
 #include <ripple/app/main/Application.h>
-#include <ripple/app/misc/OrderBook.h>
 #include <mutex>
 
 namespace ripple {
@@ -37,15 +36,13 @@ public:
     setup(std::shared_ptr<ReadView const> const& ledger);
     void
     update(std::shared_ptr<ReadView const> const& ledger);
-    void
-    invalidate();
 
     void
     addOrderBook(Book const&);
 
     /** @return a list of all orderbooks that want this issuerID and currencyID.
      */
-    OrderBook::List
+    std::vector<Book>
     getBooksByTakerPays(Issue const&);
 
     /** @return a count of all orderbooks that want this issuerID and
@@ -68,22 +65,14 @@ public:
         const AcceptedLedgerTx& alTx,
         Json::Value const& jvObj);
 
-    using IssueToOrderBook = hash_map<Issue, OrderBook::List>;
-
 private:
-    void
-    rawAddBook(Book const&);
-
     Application& app_;
 
-    // by ci/ii
+    // Maps order books by "issue in" to "issue out":
-    IssueToOrderBook mSourceMap;
+    hardened_hash_map<Issue, hardened_hash_set<Issue>> allBooks_;
-
-    // by co/io
-    IssueToOrderBook mDestMap;
 
     // does an order book to XRP exist
-    hash_set<Issue> mXRPBooks;
+    hash_set<Issue> xrpBooks_;
 
     std::recursive_mutex mLock;
 
@@ -91,7 +80,7 @@ private:
 
     BookToListenersMap mListeners;
 
-    std::uint32_t mSeq;
+    std::atomic<std::uint32_t> seq_;
 
     beast::Journal const j_;
 };

src/ripple/app/misc/NetworkOPs.cpp

@@ -3072,7 +3072,8 @@ NetworkOPsImp::pubValidatedTransaction(
                 it = mStreamMaps[sRTTransactions].erase(it);
         }
     }
-    app_.getOrderBookDB().processTxn(alAccepted, alTx, jvObj);
+    if (alTx.getResult() == tesSUCCESS)
+        app_.getOrderBookDB().processTxn(alAccepted, alTx, jvObj);
     pubAccountTransaction(alAccepted, alTx, true);
 }
 

src/ripple/app/misc/OrderBook.h

@@ -1,89 +0,0 @@
-//------------------------------------------------------------------------------
-/*
-    This file is part of rippled: https://github.com/ripple/rippled
-    Copyright (c) 2012, 2013 Ripple Labs Inc.
-
-    Permission to use, copy, modify, and/or distribute this software for any
-    purpose  with  or without fee is hereby granted, provided that the above
-    copyright notice and this permission notice appear in all copies.
-
-    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
-    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
-    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-*/
-//==============================================================================
-
-#ifndef RIPPLE_APP_MISC_ORDERBOOK_H_INCLUDED
-#define RIPPLE_APP_MISC_ORDERBOOK_H_INCLUDED
-
-#include <ripple/basics/CountedObject.h>
-
-namespace ripple {
-
-/** Describes a serialized ledger entry for an order book. */
-class OrderBook : public CountedObject<OrderBook>
-{
-public:
-    using pointer = std::shared_ptr<OrderBook>;
-    using ref = std::shared_ptr<OrderBook> const&;
-    using List = std::vector<pointer>;
-
-    /** Construct from a currency specification.
-
-        @param index ???
-        @param book in and out currency/issuer pairs.
-    */
-    // VFALCO NOTE what is the meaning of the index parameter?
-    OrderBook(uint256 const& base, Book const& book)
-        : mBookBase(base), mBook(book)
-    {
-    }
-
-    uint256 const&
-    getBookBase() const
-    {
-        return mBookBase;
-    }
-
-    Book const&
-    book() const
-    {
-        return mBook;
-    }
-
-    Currency const&
-    getCurrencyIn() const
-    {
-        return mBook.in.currency;
-    }
-
-    Currency const&
-    getCurrencyOut() const
-    {
-        return mBook.out.currency;
-    }
-
-    AccountID const&
-    getIssuerIn() const
-    {
-        return mBook.in.account;
-    }
-
-    AccountID const&
-    getIssuerOut() const
-    {
-        return mBook.out.account;
-    }
-
-private:
-    uint256 const mBookBase;
-    Book const mBook;
-};
-
-}  // namespace ripple
-
-#endif

src/ripple/app/paths/Pathfinder.cpp

@@ -1126,16 +1126,14 @@ Pathfinder::addLink(
                 if (continueCallback && !continueCallback())
                     return;
                 if (!currentPath.hasSeen(
-                        xrpAccount(),
+                        xrpAccount(), book.out.currency, book.out.account) &&
-                        book->getCurrencyOut(),
+                    !issueMatchesOrigin(book.out) &&
-                        book->getIssuerOut()) &&
-                    !issueMatchesOrigin(book->book().out) &&
                     (!bDestOnly ||
-                     (book->getCurrencyOut() == mDstAmount.getCurrency())))
+                     (book.out.currency == mDstAmount.getCurrency())))
                 {
                     STPath newPath(currentPath);
 
-                    if (book->getCurrencyOut().isZero())
+                    if (book.out.currency.isZero())
                     {  // to XRP
 
                         // add the order book itself
@@ -1158,9 +1156,9 @@ Pathfinder::addLink(
                             incompletePaths.push_back(newPath);
                     }
                     else if (!currentPath.hasSeen(
-                                 book->getIssuerOut(),
+                                 book.out.account,
-                                 book->getCurrencyOut(),
+                                 book.out.currency,
-                                 book->getIssuerOut()))
+                                 book.out.account))
                     {
                         // Don't want the book if we've already seen the issuer
                         // book -> account -> book
@@ -1173,8 +1171,8 @@ Pathfinder::addLink(
                                 STPathElement::typeCurrency |
                                     STPathElement::typeIssuer,
                                 xrpAccount(),
-                                book->getCurrencyOut(),
+                                book.out.currency,
-                                book->getIssuerOut());
+                                book.out.account);
                         }
                         else
                         {
@@ -1183,19 +1181,19 @@ Pathfinder::addLink(
                                 STPathElement::typeCurrency |
                                     STPathElement::typeIssuer,
                                 xrpAccount(),
-                                book->getCurrencyOut(),
+                                book.out.currency,
-                                book->getIssuerOut());
+                                book.out.account);
                         }
 
                         if (hasEffectiveDestination &&
-                            book->getIssuerOut() == mDstAccount &&
+                            book.out.account == mDstAccount &&
-                            book->getCurrencyOut() == mDstAmount.getCurrency())
+                            book.out.currency == mDstAmount.getCurrency())
                         {
                             // We skipped a required issuer
                         }
                         else if (
-                            book->getIssuerOut() == mEffectiveDst &&
+                            book.out.account == mEffectiveDst &&
-                            book->getCurrencyOut() == mDstAmount.getCurrency())
+                            book.out.currency == mDstAmount.getCurrency())
                         {  // with the destination account, this path is
                            // complete
                             JLOG(j_.trace())
@@ -1210,9 +1208,9 @@ Pathfinder::addLink(
                                 newPath,
                                 STPathElement(
                                     STPathElement::typeAccount,
-                                    book->getIssuerOut(),
+                                    book.out.account,
-                                    book->getCurrencyOut(),
+                                    book.out.currency,
-                                    book->getIssuerOut()));
+                                    book.out.account));
                         }
                     }
                 }