Improve page deletion from multipage directories (RIPD-1353) #1923

nbougalis · 2016-12-06T08:08:03Z

This pull request does not change how page entries are sorted or how pages are selected. An amendment will be introduced in the future.

seelabs · 2016-12-06T15:39:15Z

#include <ripple/dedger/Directory.h> in View.cpp to fix the non-unity builds.

The AccountInfo unit test is hitting an assert on line 311 of Indexes.cpp. The root.type was ltANY, the assert expects ltDIR_NODE. I haven't dug into it further.

vinniefalco · 2016-12-06T15:57:44Z

src/ripple/ledger/ApplyView.h

+    dirRemove (
+        Keylet const& directory,
+        std::uint64_t page,
+        Keylet const& key,


Why are there two of these?

One takes a Keylet but Sometimes we don't have a Keylet available, so we offer the other version which accepts a uint256. We could have required callers to construct the Keylet but I wanted to minimize changes for this round.

I will be removing uses of the old API along with the amendment to enable sorting of owner directories, so that will go away.

It was never the intention of Keylet to be used as a function parameter

Sorry, that was misleading. Keylet as function parameter is fine. There is no computation performed in the Keylet constructor.

nbougalis · 2016-12-06T16:19:30Z

Thanks @seelabs. I forgot to push a FOLD commit that addressed this. It's coming.

vinniefalco · 2016-12-06T23:34:45Z

src/ripple/ledger/impl/ApplyView.cpp

+// Called when the owner count changes
+// This is required to support PaymentSandbox
+void
+ApplyView::adjustOwnerCountHook (


There was no need to put this in the .cpp file it was more concise in the header. This adds additional lines with no corresponding benefit. Same for the other empty function.

vinniefalco · 2016-12-06T23:35:27Z

src/ripple/app/tx/impl/Transactor.cpp

@@ -563,7 +564,7 @@ void removeUnfundedOffers (ApplyView& view, std::vector<uint256> const& offers,
        {
            // offer is unfunded
            offerDelete (view, sleOffer, viewJ);
-            if (++removed == 1000)
+            if (++removed == unfundedOfferRemoveLimit)


Paranoia, consider:

if(++removed >= unfundedOfferRemoveLimit)

vinniefalco · 2016-12-06T23:35:53Z

src/ripple/ledger/ApplyView.h

+        @param describe callback to add required entries to a new page
+    */
+    /** @{ */
+    std::pair<bool, std::uint64_t>


The return value needs to be documented

vinniefalco · 2016-12-06T23:36:05Z

src/ripple/ledger/ApplyView.h

+            the directory itself.
+    */
+    /** @{ */
+    bool


The return value needs to be documented

vinniefalco · 2016-12-06T23:38:17Z

src/ripple/ledger/impl/ApplyView.cpp

+        node = peek (keylet::page(directory, page));
+
+        if (!node)
+            LogicError ("Directory chain: root back-pointer broken.");


Informational: We are creating code that is guaranteed not to be included in coverage reports.

The irony that I am the one who introduced this idiom is not lost on me.

vinniefalco · 2016-12-06T23:40:59Z

src/ripple/ledger/impl/ApplyView.cpp

+        return false;
+
+    {
+        auto entries = node->getFieldV256(sfIndexes);


Does this make a copy?

It does. We can get away with it with a bit of ugliness:

auto entries = dynamic_cast<STVector256&>(node->getField(sfIndexes));

vinniefalco · 2016-12-06T23:43:30Z

src/test/ledger/Directory_test.cpp

+            auto const dir = Dir(*env.current(),
+                keylet::ownerDir(bob));
+
+            auto iter = std::begin(dir);


Frivolous, dir.begin() is idiomatic.

vinniefalco · 2016-12-06T23:44:58Z

src/test/ledger/Directory_test.cpp

-} // test
-} // ripple
+}
+}


Solid looking test

vinniefalco · 2016-12-06T23:45:57Z

Looks good, minor style nits but mergeable.
👍

seelabs · 2016-12-09T19:40:04Z

src/ripple/ledger/impl/ApplyView.cpp

+    if (indexes.size () < dirNodeMaxEntries)
+    {
+        // If we're not maintaining strict order, then
+        // sort entries lexicographically.


This comment is misleading. We don't sort the entries, we insert at the correct position if the vector was already sorted.

seelabs · 2016-12-09T19:43:47Z

src/ripple/ledger/impl/ApplyView.cpp

+                LogicError ("dirInsert: double insertion");
+
+            v.insert (pos, key);
+            indexes = v;


We can save two copies of the vector if we add an insertmember to STVector. We'd save the copy on line 96 (it would becore auto const& v = ...), and the copy here. Since STVector already has an erase member insert seems OK.

It also has a push_back so insert seems reasonable.

seelabs · 2016-12-09T19:45:26Z

src/ripple/ledger/impl/ApplyView.cpp

+
+    // Insert the new key:
+    indexes.clear();
+    indexes.push_back (key);


Nit: I'd move this code down closer to where it's used, around line 136.

seelabs · 2016-12-09T19:50:56Z

src/ripple/ledger/impl/ApplyView.cpp

+        if (entries.end () == it)
+            return false;
+
+        // We always preserve the relative order when we remove.


I think I need to understand why we're starting to care about order in non-book directories (I've forgotten).

If we don't need to preserve order (owner directories) there's an optimization that saves some copying: copy the last element to the current element then resize the vector.

We had this optimization in the existing code: we want to have the ability to sort pages going forward.

seelabs · 2016-12-09T19:56:05Z

src/ripple/ledger/impl/ApplyView.cpp

+    update(next);
+
+    // The page is no longer linked. Delete it.
+    erase(node);


At this point we need to check if there are two pages left and the last page is empty. If we don't, we won't handle the [EMPTY] -> [Just removed last element] -> [EMPTY] scenario. I'm guessing the unit tests pass because the last page is no longer allowed to be empty. It it were (i.e. an existing ledger), then I would expect the unit test to fail.

I've addressed this in 4da5382

seelabs · 2016-12-09T19:56:23Z

src/ripple/ledger/impl/ApplyView.cpp

+    // deleted, and, if so, whether the entire directory
+    // can now be removed.
+    auto prevPage = node->getFieldU64(sfIndexPrevious);
+    auto nextPage = node->getFieldU64(sfIndexNext);


Nit: const

Can't: we have this further down:

// Make sure our local values reflect the // updated information: nextPage = currPage; prevPage = currPage;

seelabs · 2016-12-09T20:02:43Z

src/ripple/ledger/impl/View.cpp

-    JLOG (j.trace()) << "dirAdd:" <<
-        " dir=" << to_string (dir.key) <<
-        " uLedgerIndex=" << to_string (uLedgerIndex);
+    bool existed = static_cast<bool>(view.peek (dir));


Nit: const

seelabs · 2016-12-09T20:06:32Z

src/ripple/ledger/impl/View.cpp

-    uNodeDir = sleRoot->getFieldU64 (sfIndexPrevious);       // Get index to last directory node.
+static
+boost::optional<std::uint64_t>
+findPage (


This is called findPage, but it only searches the first 20 pages and we have to look at the code to see that. I'm worried someone is going to reuses this and it's going to bite us. Suggestions (any are fine with me):

Put the code back inside dirDelete, either with a lambda or just embed the code

Add a parameter to the function that controls how many pages to search

Add a comment documenting that it only searches 20 pages

findPage has been integrated directly into dirDelete

seelabs · 2016-12-09T20:08:55Z

src/ripple/ledger/impl/View.cpp

+    // Emulate the old semantics: if uNodeDir isn't hard
+    // and fast and there's no page number, search for the
+    // item:
+    if (bSoft && uNodeDir == 0)


This code searches the first 20 pages of a directory, but only when uNodeDir == 0. The code this replaces would search from the current page up to the 20th page (i.e. old code would search even if uNodeDir was not zero).

Good eyes. However, I believe that all the use cases specified 0 when bSoft == true, so the code correctly replicates the semantics.

seelabs · 2016-12-09T20:14:22Z

As you mention in your commit message, this is tx breaking, so it can't be merged without an amendment.

nbougalis · 2016-12-09T20:23:26Z

Updating to address comments. Thanks.

JoelKatz · 2016-12-12T20:07:12Z

Looks good so far. I'll wait for the final update before I approve it.

vinniefalco · 2016-12-16T16:08:30Z

src/ripple/app/tx/impl/CreateTicket.cpp

    }

-    return result.first;
+    sleTicket->setFieldU64(sfOwnerNode, result.second);


Why not

(*sleTicket)[sfOwnerNode] = result.second;

?

vinniefalco · 2016-12-16T16:09:49Z

src/ripple/ledger/ApplyView.h

+        std::function<void(std::shared_ptr<SLE> const&)> describe);
+    /** @} */
+
+    /** Removes an entry from a directory


Use imperative voice, e.g. "Remove an entry from a directory"

vinniefalco · 2016-12-16T16:10:47Z

src/ripple/ledger/impl/ApplyView.cpp

+    // Save some space by not specifying the value 0 since
+    // it's the default.
+    if (page != 1)
+        node->setFieldU64 (sfIndexPrevious, page - 1);


Can we write (*node)[sfIndexPrevious] = page - 1?

Yes. Will fix.

codecov-io · 2016-12-19T19:53:13Z

Current coverage is 65.37% (diff: 77.82%)

Merging #1923 into develop will decrease coverage by 0.09%

@@            develop      #1923   diff @@
==========================================
  Files           698        699     +1   
  Lines         49745      49755    +10   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          32568      32526    -42   
- Misses        17177      17229    +52   
  Partials          0          0

Powered by Codecov. Last update 9e960ff...33a58af

seelabs

Left a couple nits, but looks good. I think this is ready for a thumbs up once it's on an amendment.

seelabs · 2016-12-20T16:44:35Z

src/ripple/app/tx/impl/CreateTicket.cpp

-    return result.first;
+    (*sleTicket)[sfOwnerNode] = result.second;
+
+    // If we succeeded, the new entry counts agains the


Typo: agains -> against

seelabs · 2016-12-20T16:46:46Z

src/ripple/ledger/impl/ApplyView.cpp

+        if (!strictOrder)
+        {
+            // We can't use std::lower_bound here because
+            // existing pages may not be fully sorted.


"Not fully sorted" implies there is some order to to page, but we can't make any guarantees about the order at all. The pages are just "not sorted".

seelabs · 2016-12-20T16:53:47Z

src/ripple/ledger/impl/ApplyView.cpp

+            if (pos != indexes.end() && key == *pos)
+                LogicError ("dirInsert: double insertion");
+
+            indexes.insert (pos, key);


Someone is going to look at this code and wonder why it's being inserted in this order. However, there is no functional benefit now. We are inserting in this order because in the future we plan on introducing sorting to certain directories, correct?

I'd at least add a comment explaining why we are inserting this way.

nbougalis · 2016-12-20T20:38:30Z

An amendment is needed for this, so I'm going to close it for now, then rebase, add the amendment and resubmit it.

Improve page deletion from multipage directories (RIPD-1353)

d4727f0

nbougalis assigned JoelKatz, vinniefalco and seelabs Dec 6, 2016

vinniefalco reviewed Dec 6, 2016

View reviewed changes

src/ripple/ledger/ApplyView.h

the directory itself.

*/

/** @{ */

bool

Copy link

Contributor

vinniefalco Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value needs to be documented

vinniefalco reviewed Dec 6, 2016

View reviewed changes

src/test/ledger/Directory_test.cpp

} // test

} // ripple

}

}

Copy link

Contributor

vinniefalco Dec 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid looking test

seelabs reviewed Dec 9, 2016

View reviewed changes

[FOLD] Make this compile in unity

9657a9f

nbougalis added 2 commits December 15, 2016 18:18

[FOLD] Address review comments, reduce public API

4da5382

[FOLD] constify two variables

2094b80

vinniefalco reviewed Dec 16, 2016

View reviewed changes

nbougalis added 3 commits December 17, 2016 13:16

[FOLD] use new syntax for field access

145752c

[FOLD] Remove unused macro

4eebf7b

[FOLD] Fix bool return

33a58af

seelabs reviewed Dec 20, 2016

View reviewed changes

nbougalis closed this Dec 20, 2016

Improve page deletion from multipage directories (RIPD-1353) #1923

Improve page deletion from multipage directories (RIPD-1353) #1923

Conversation

nbougalis commented Dec 6, 2016

seelabs commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinniefalco commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs commented Dec 9, 2016

nbougalis commented Dec 9, 2016

JoelKatz commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 19, 2016

Current coverage is 65.37% (diff: 77.82%)

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis commented Dec 20, 2016

nbougalis Dec 14, 2016 •

edited

Loading

seelabs Dec 9, 2016 •

edited

Loading