Base uint2 #3646

nbougalis · 2020-10-26T05:08:28Z

This PR builds on top of #3640 (the bottom-most commits are from there) and involves a refactoring and general code cleanup of several classes involved with SHAMaps. In addition to improving memory usage by 8 bytes per SHAMapAbstractNode, the changes also improve type safety by having distinct classes for each type of leaf we have, instead of a single class.

Each change is a separate commit (hence the volume of commits). Since this is critical code, I wanted to make sure that the changes were small enough to be easy to verify by inspection. For the same reason, I (mostly) avoided running clang-format and expect that there are several violations.

This is complex code and it can be challenging to grok, especially since the rationale behind some of these decisions exists only in the heads of three people: Arthur, David and myself. To that end, I made several improvements to the documentation as well, which should help make this code more tractable for others. Honestly, this is a WIP.

You only need to review the commits tagged [WIP] but please review them VERY CAREFULLY. If you're even slightly unsure that a change is correct, or if something is unclear, please leave a comment.

nbougalis · 2020-10-26T15:53:39Z

src/ripple/app/misc/SHAMapStoreImp.cpp

-        node.getNodeHash().as_uint256(), node.getSeq());
+    // FIXME: node.owner() is the wrong thing to pass: fetchNodeObject expects
+    //        a ledger sequence, not a copy-on-write identifier.
+    dbRotating_->fetchNodeObject(node.getHash().as_uint256(), node.owner());


After mentioning this bug in chat, it was fixed by @miguelportilla in #3643 so it will be resolved in a different PR, and this comment will go away during the merge/conflict resolution.

scottschurr

I reviewed this pull request carefully, one commit at a time. As far as I can see everything is in order.

I have two further suggestions for this pull request.

I think now would be good time to rename SHAMapTreeNode to SHAMapLeafNode. That seems to me to be what the class represents.
I really like the way you renamed SHAMap::seq_ to cowid_ Similarly, the rename of member variable SHAMapAbstractNode::mSeq to SHAMapAbstractNode::cowid_ is a good choice. However I think the member function SHAMapAbstractNode::owner() (which returns cowid_) would be better named SHAMapAbstractNode::cowid(). The value is easier to track and audit when it does not change names in the middle of usage. Similarly, I think the parameters of SHAMap*Node member functions named owner would be easier to follow if they were renamed cowid.

Neither of these suggests affect correctness, but I think they will make the code easier to follow.

scottschurr · 2020-10-27T19:30:29Z

src/ripple/shamap/impl/SHAMapSync.cpp

+            // Inner nodes must be at a level strictly less than 64
+            // but leaf nodes (while notionally at level 64) can be
+            // at any depth:
+            if (newNode->isInner() && iNodeID.getDepth() == 64)


Nice comment! It leads me to ask the question of whether it is valid for any node to have a depth greater than 64. I wonder whether this might be a better check:

// Inner nodes must be at a level strictly less than 64 // but leaf nodes (while notionally at level 64) can be // at any depth up to 64: if (iNodeID.getDepth() > 64 || (newNode->isInner() && iNodeID.getDepth() == 64)) { // Map is provably invalid state_ = SHAMapState::Invalid; return SHAMapAddNode::useful(); }

Good idea, but note that a previous set of changes has made it impossible to construct such a node; or rather, the constructor of the object has an assert that enforces this invariant. Still, an extra (basically free) check here seems like a good idea.

scottschurr · 2020-10-28T22:21:56Z

src/ripple/shamap/SHAMapTreeNode.h

+    {
+    }
+
+    explicit SHAMapAbstractNode(std::uint32_t seq, SHAMapHash const& hash)


The explicit does no harm here, but it's not really useful since this constructor takes more than one argument.

@scottschurr just to be pedantic, putting explicit on multi-parameter constructors prevents them from being initialized with braces. I.e. explicit Foo(int, int) prevents a Foo from being initialized with {1,2}. I'd probably prefer we keep the explicit, but I don't feel that strongly about it (and in fact I often leave it off multi-paramter constructors myself). One place this would matter is when a function takes the class as a parameter and it is called with braces. I.e the explicit two param constructor prevents bar(Foo f) from being called as bar({1,2}).

@seelabs, thanks for that correction. Excellent point.

src/ripple/app/consensus/RCLConsensus.cpp

src/ripple/shamap/impl/SHAMap.cpp

scottschurr · 2020-10-29T17:44:35Z

src/ripple/shamap/SHAMapTreeNode.h

-    getType() const override
-    {
-        return mType;
-    }

    bool
    isLeaf() const override


Consider marking as final those virtual methods in this class that are not supposed to be overridden by further derived classes. In the current final state of this branch I believe those methods are isLeaf(), isInner(), invariants(), and getString().

Reopening this so this comment is seen. Marking these function final without the override is causing gcc8 to warn; we need both the final and the override. However, some of these classes are already marked as final. For final classes, I would not mark these functions as final (just to cut down on the noise of final override). I'll make a patch to resolve the warnings.

@nbougalis
Here's a patch to reformat with clang-format: seelabs@e9c5db5
Here's a patch to resolve the suggest-override warnings: seelabs@814a529

Hopefully this will resolve the CI issues. Also note, I'm not able to test those locally, as I do not have access to gcc 8 (gcc 9 and up do not warn on this).

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/shamap/SHAMap.h

src/ripple/app/ledger/impl/LedgerMaster.cpp

scottschurr · 2020-10-31T01:05:15Z

src/ripple/overlay/Message.h

@@ -64,6 +64,10 @@ class Message : public std::enable_shared_from_this<Message>
        int type,
        boost::optional<PublicKey> const& validator = {});

+    /** Retrieve the size of the packed but uncompressed message data. */
+    std::size_t
+    getBufferSize();


getBufferSize() const?

src/ripple/app/ledger/impl/LedgerMaster.cpp

seelabs

👍 Very nice set of cleanups. We do need to reformat this with clang-format before it's merged tho.

src/ripple/app/ledger/impl/InboundLedgers.cpp

src/ripple/app/ledger/impl/LedgerMaster.cpp

src/ripple/shamap/impl/SHAMap.cpp

HowardHinnant · 2020-11-05T20:02:21Z

src/ripple/basics/base_uint.h

-
-        if ((pEnd - pBegin) & 1)
-            *pOut++ = charUnHex(*pBegin++);
+            throw c;


I like this improved API for base_uint, but I don't recommend using exceptions for flow control. It is expensive both in terms of performance and code size. Furthermore the static_cast is in the wrong place to be effective since the expression (hi << 4) + lo promotes everything back to int.

Here is what I recommend:

[[nodiscard]] bool parseHex(std::string_view sv) { zero(); if (sv == "0") return true; if (sv.size() != bytes * 2) return false; auto out = data(); auto in = sv.begin(); while (in != sv.end()) { auto const hi = charUnHex(*in++); auto const lo = charUnHex(*in++); if (hi == -1 || lo == -1) return false; *out++ = static_cast<value_type>((hi << 4) + lo); } return true; }

scottschurr

Looks great! I left a few notes on the changes since my last review. Also, I think Message::getBufferSize() could be const. But none of the notes I left are mandatory. Thanks for your work here!

scottschurr · 2020-11-05T20:23:51Z

src/ripple/shamap/README.md


-SHAMapTreeNode publicly inherits directly from SHAMapAbstractNode.  It holds the
+SHAMapLeafNode publicly inherits directly from SHAMapAbstractNode.  It holds the
 following data:

 1.  A shared_ptr to a const SHAMapItem.


I can't comment on the section in question (since it didn't change), but I looked through the section starting on line 326, SHAMap Improvements. Are there any changes suggested by that section which have not been done yet? Would it make sense to remove that section?

scottschurr · 2020-11-05T20:30:19Z

src/ripple/shamap/SHAMapTreeNode.h

@@ -196,7 +196,7 @@ class SHAMapAbstractNode

    /** Make a copy of this node, setting the owner. */


Does it make sense to change "owner" to "cowid" in this comment?

scottschurr · 2020-11-05T20:32:20Z

src/ripple/shamap/SHAMapTreeNode.h

@@ -337,36 +338,36 @@ class SHAMapInnerNode : public SHAMapAbstractNode,

 // SHAMapTreeNode represents a leaf, and may eventually be renamed to reflect


This comment looks obsolete.

scottschurr · 2020-11-05T20:38:03Z

src/ripple/shamap/SHAMapTreeNode.h

        @param hash The hash associated with this node, if any.
     */
    /** @{ */
-    explicit SHAMapAbstractNode(std::uint32_t owner) noexcept : cowid_(owner)
+    explicit SHAMapAbstractNode(std::uint32_t cowid) noexcept : cowid_(cowid)


The name of this file, SHAMapTreeNode, no longer represents any class in the code base. You could consider renaming the file to SHAMapAbstractNode. But I think maybe a better idea would be to rename the SHAMapAbstractNode class to SHAMapTreeNode, since that is precisely what a SHAMapAbstractNode is.

Not a requirement, just a thought.

scottschurr · 2020-11-05T20:40:43Z

src/ripple/shamap/SHAMapTreeNode.h

@@ -568,7 +572,7 @@ SHAMapInnerNode::setFullBelowGen(std::uint32_t gen)

 // SHAMapTreeNode


This comment needs to be updated.

src/ripple/app/ledger/impl/LedgerMaster.cpp

scottschurr · 2020-11-05T21:50:57Z

src/ripple/shamap/README.md

@@ -17,14 +17,14 @@ or account state can be used to navigate the trie.
 A `SHAMap` is a trie with two node types:

 1.  SHAMapInnerNode


This README.md still contains a few references to SHAMapTreeNode. Are those intentional, or should they be expunged?

seelabs · 2020-11-13T03:33:48Z

The CI issues need to be resolved as well (mostly looks like missing overrides).

Edit: I do not see these warnings on my local machine. The difference seems to be gcc 10 does not warn about missing overrides when you include final (correctly, IMHO). But gcc 8, which we use for CI does warn.

The existing SHAMapNodeID object has both a valid and an invalid state and requirs callers to verify the state of an instance prior to using it. A simple set of changes removes that restriction and ensures that all instances are valid, making the code more robust. This change also: 1. Introduces a new function to construct a SHAMapNodeID from a serialized blob; and 2. Reduces the amount of constructors the class exposes.

- Provide separate functions for serializing depending on whether one wants a "wire" version of a node, or one suitable for hashing. - Remove unused functions

- Simplify and consolidate code for parsing hex input. - Replace beast::endian::order with boost::endian::order. - Simplify CountedObject code. - Remove pre-C++17 workarounds in favor of C++17 based solutions. - Improve `base_uint` and simplify its hex-parsing interface by consolidating the `SexHex` and `SetHexExact` methods into one API: `parseHex` which forces callers to verify the result of the operation; as a result some public-facing API endpoints may now return errors when passed values that were previously accepted. - Remove the simple fallback implementations of SHA2 and RIPEMD introduced to reduce our dependency on OpenSSL. The code is slow and rarely, if ever, exercised and we rely on OpenSSL functionality for Boost.ASIO as well.

The Travis build matrix included builds for gcc 7 and clang 7 which do not appear to have sufficient C++17 support to compile rippled any longer.

This commit combines a number of cleanups, targeting both the code structure and the code logic. Large changes include: - Using more strongly-typed classes for SHAMap nodes, instead of relying on runtime-time detection of class types. This change saves 16 bytes of memory per node. - Improving the interface of SHAMap::addGiveItem and SHAMap::addItem to avoid the need for passing two bool arguments. - Documenting the "copy-on-write" semantics that SHAMap uses to efficiently track changes in individual nodes. - Removing unused code and simplifying several APIs. - Improving function naming.

seelabs · 2020-11-17T22:06:57Z

We've got a linker error on mac:

Undefined symbols for architecture x86_64:

  "std::__1::__itoa::__u64toa(unsigned long long, char*)", referenced from:

      std::__1::__itoa::__traits_base<unsigned long long, void>::__convert(unsigned long long, char*) in libxrpl_core.a(unity_6_cxx.cxx.o)

HowardHinnant · 2020-11-17T22:40:27Z

This comes from a new use of std::to_chars and does not happen except on platforms where the macOS is older than current-3 (I'm testing on current-3 and am linking). Apple simply does not ship std::to_chars on the older versions of macOS.

I think this is safe to ignore.

scottschurr · 2020-11-17T23:23:55Z

Great continues to look even better. I found one last remaining reference to SHAMapAbstractNode in a comment in SHAMap.cpp. It looks like this:

    @note The node must have already been unshared by having the caller
          first call SHAMapAbstractNode::share().

I think if you change that to SHAMapTreeNode::share() you'll have it all cleaned out.

seelabs · 2020-11-19T14:53:11Z

@nbougalis This patch should fix the CI linker error: eae4367

nbougalis · 2020-11-22T23:34:34Z

I've addressed all remaining concerns.

seelabs

@nbougalis The mac linker errors still appear to be there. We need the patch referenced in this comment: #3646 (comment)

Edit: I see it's been added to the rollup PR. NVM.

gregtatcam

LGTM 👍

nbougalis added Tech Debt Non-urgent improvements Documentation README changes, code comments, etc. labels Oct 26, 2020

nbougalis requested review from gregtatcam, seelabs, HowardHinnant and scottschurr October 26, 2020 05:08

nbougalis assigned gregtatcam, seelabs, HowardHinnant and scottschurr Oct 26, 2020

nbougalis commented Oct 26, 2020

View reviewed changes

scottschurr approved these changes Nov 2, 2020

View reviewed changes

seelabs approved these changes Nov 3, 2020

View reviewed changes

src/ripple/app/ledger/impl/InboundLedgers.cpp Outdated Show resolved Hide resolved

src/ripple/app/ledger/impl/LedgerMaster.cpp Show resolved Hide resolved

src/ripple/shamap/impl/SHAMap.cpp Show resolved Hide resolved

HowardHinnant reviewed Nov 5, 2020

View reviewed changes

scottschurr approved these changes Nov 5, 2020

View reviewed changes

HowardHinnant approved these changes Nov 13, 2020

View reviewed changes

nbougalis mentioned this pull request Nov 14, 2020

Severa improvements / simplifications to the codebase #3640

Closed

nbougalis added 7 commits November 14, 2020 03:40

Simplify SHAMapTreeNode APIs:

c8aa833

- Provide separate functions for serializing depending on whether one wants a "wire" version of a node, or one suitable for hashing. - Remove unused functions

Track counts of SHAMap-related objects

6617997

Update Travis build matrix:

96a23a2

The Travis build matrix included builds for gcc 7 and clang 7 which do not appear to have sufficient C++17 support to compile rippled any longer.

Simplify processing of inbound ledger data

8ba7a4b

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 18, 2020

nbougalis removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 18, 2020

nbougalis mentioned this pull request Nov 23, 2020

Rollup (3603, 3646 and 3672) #3675

Closed

nbougalis linked an issue Nov 23, 2020 that may be closed by this pull request

Refactor base_uint SetHex and SetHexExact methods #3507

Open

seelabs reviewed Nov 23, 2020

View reviewed changes

gregtatcam approved these changes Nov 29, 2020

View reviewed changes

nbougalis mentioned this pull request Dec 4, 2020

Proposed 1.7.0-b5 #3684

Merged

carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 4, 2020

nbougalis closed this in #3684 Dec 5, 2020

nbougalis deleted the base_uint2 branch October 16, 2023 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base uint2 #3646

Base uint2 #3646

nbougalis commented Oct 26, 2020 •

edited

Loading

nbougalis Oct 26, 2020

scottschurr left a comment

scottschurr Oct 27, 2020

nbougalis Nov 3, 2020 •

edited

Loading

scottschurr Oct 28, 2020

seelabs Nov 2, 2020 •

edited

Loading

scottschurr Nov 2, 2020

scottschurr Oct 29, 2020

seelabs Nov 13, 2020

seelabs Nov 13, 2020 •

edited

Loading

scottschurr Oct 31, 2020

seelabs left a comment •

edited

Loading

HowardHinnant Nov 5, 2020

scottschurr left a comment

scottschurr Nov 5, 2020

nbougalis Nov 14, 2020

scottschurr Nov 5, 2020

nbougalis Nov 14, 2020

scottschurr Nov 5, 2020

scottschurr Nov 5, 2020

scottschurr Nov 5, 2020

scottschurr Nov 5, 2020

seelabs commented Nov 13, 2020 •

edited

Loading

seelabs commented Nov 17, 2020

HowardHinnant commented Nov 17, 2020

scottschurr commented Nov 17, 2020

seelabs commented Nov 19, 2020

nbougalis commented Nov 22, 2020

seelabs left a comment •

edited

Loading

gregtatcam left a comment

		@@ -196,7 +196,7 @@ class SHAMapAbstractNode

		/** Make a copy of this node, setting the owner. */

		@@ -337,36 +338,36 @@ class SHAMapInnerNode : public SHAMapAbstractNode,

		// SHAMapTreeNode represents a leaf, and may eventually be renamed to reflect

		@@ -568,7 +572,7 @@ SHAMapInnerNode::setFullBelowGen(std::uint32_t gen)

		// SHAMapTreeNode

		@@ -17,14 +17,14 @@ or account state can be used to navigate the trie.
		A `SHAMap` is a trie with two node types:

		1. SHAMapInnerNode

Base uint2 #3646

Base uint2 #3646

Conversation

nbougalis commented Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Nov 2, 2020 • 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

seelabs Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr 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

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 Nov 13, 2020 • edited Loading

seelabs commented Nov 17, 2020

HowardHinnant commented Nov 17, 2020

scottschurr commented Nov 17, 2020

seelabs commented Nov 19, 2020

nbougalis commented Nov 22, 2020

seelabs left a comment • edited Loading

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

nbougalis commented Oct 26, 2020 •

edited

Loading

nbougalis Nov 3, 2020 •

edited

Loading

seelabs Nov 2, 2020 •

edited

Loading

seelabs Nov 13, 2020 •

edited

Loading

seelabs left a comment •

edited

Loading

seelabs commented Nov 13, 2020 •

edited

Loading

seelabs left a comment •

edited

Loading