Tidy RangeSet #2113

bachase · 2017-05-10T12:35:35Z

@wufeipeng fix an issue in #2109 in the unused getPrev function of RangeSet. This is an alternate fix, which eliminates several unused members of RangeSet and increases test coverage of the remaining member functions.

codecov-io · 2017-05-10T13:12:58Z

Codecov Report

Merging #2113 into develop will increase coverage by <.01%.
The diff coverage is 32.71%.

@@             Coverage Diff             @@
##           develop    #2113      +/-   ##
===========================================
+ Coverage    68.98%   68.98%   +<.01%     
===========================================
  Files          684      683       -1     
  Lines        50461    50369      -92     
===========================================
- Hits         34812    34749      -63     
+ Misses       15649    15620      -29

Impacted Files	Coverage Δ
src/ripple/app/ledger/LedgerMaster.h	`83.33% <ø> (ø)`	⬆️
src/ripple/app/ledger/impl/LedgerMaster.cpp	`43.73% <15.47%> (-0.31%)`	⬇️
src/ripple/basics/RangeSet.h	`95.65% <95.65%> (-4.35%)`	⬇️
src/ripple/basics/impl/ResolverAsio.cpp	`92.75% <0%> (-0.73%)`	⬇️
src/ripple/protocol/impl/Serializer.cpp	`70.3% <0%> (+0.34%)`	⬆️
src/ripple/server/impl/BaseWSPeer.h	`68.12% <0%> (+0.62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bfd9de...32d9e4f. Read the comment docs.

mellery451 · 2017-05-10T17:42:39Z

src/test/basics/RangeSet_test.cpp

@@ -26,69 +26,132 @@ namespace ripple {
 class RangeSet_test : public beast::unit_test::suite
 {
 public:
-    RangeSet createPredefinedSet ()
+    void
+    testSetAndHas()


these are good tests - the only other thing I'd like to see is setRange with values that overlap and the front/back/middle of an existing range. If that's already there and I missed it, then 👍

Good suggestion. I will add.

Thanks @mellery451, your suggestion uncovered a bug. I'm actually now more tempted to look at Boost ICL after all to avoid these type of errors when rolling our own.

mellery451

LGTM

ximinez

Removed code is code that can't break! 👍

bachase · 2017-05-11T14:39:31Z

Sorry to do this to you @ximinez and @mellery451, but I decided to switch to boost after that last bug was uncovered. I choose to haveRangeSet class use an interval_set internally to keep the LedgerMaster code the same.

ximinez · 2017-05-11T14:40:24Z

I don't mind a bit! I'll check it out ASAP.

ximinez

Nice abstraction. 👍

mellery451

👍 I like it

bachase · 2017-05-11T15:31:37Z

Squash and passed, thanks!

nbougalis

A couple of minor nits.

In looking at this, I started wondering if we should rethink this interface. I'll comment for posterity, but no action is needed for these:

First of all, getFirstandprevMissingreturn thisabsentvalue, and I was thinking maybeboost::optionalwould be better, but then just looking at the way we usegetFirstandprevMissing` I'm not sure that's right.

getFirst is a single-use function that leverages the weird semantics of absent being larger than any possible value. Instead the caller should check if the range is empty, before calling getFirst.

prevMissing again has this weird check for 0 and returns absent.

nbougalis · 2017-05-17T01:47:31Z

src/ripple/basics/impl/RangeSet.cpp

-        if (it.first == it.second)
-            ret += beast::lexicalCastThrow <std::string> ((it.first));
+        if (it.first() == it.last())
+            ret += beast::lexicalCastThrow<std::string>((it.first()));


Should we just use std::to_string here and do away with this beast::lexicalCastThrow<std::string> nonsense?

nbougalis · 2017-05-17T02:01:41Z

src/ripple/basics/RangeSet.h

 class RangeSet
 {
 public:
-    static const std::uint32_t absent = static_cast <std::uint32_t> (-1);
+    static const std::uint32_t absent = static_cast<std::uint32_t>(-1);


Uh-oh... my spidey sense is tingling! I don't like this. At a minimum, let's make it std::numeric_limits<std::uin32_t>::max.

nbougalis · 2017-05-17T10:46:56Z

src/test/basics/RangeSet_test.cpp

+        for (int i = 1; i <= 10; ++i)
+        {
+            if(!BEAST_EXPECT(r.hasValue(i)))
+                std::cout << i << "\n";


std::cout in a unit test?

D'oh. Forgot to tidy that.

nbougalis · 2017-05-17T10:49:38Z

src/test/basics/RangeSet_test.cpp


-            BEAST_EXPECT(set.prevMissing (i) == expectedPrevMissing);
+            BEAST_EXPECT(set.prevMissing(i) == expectedPrevMissing);


So, prevMissing returns a std::uint32_t (which may be initialized to static_int<std::uint32_t>(-1)) and expectedPrevMissing is an int. This smells.

nbougalis · 2017-05-17T10:52:03Z

src/ripple/basics/RangeSet.h


    // Add an item to the set
-    void setValue (std::uint32_t);
+    void setValue(std::uint32_t);


I know you went out of your way to not rename things, but since you're here, I think that some renaming makes sense: I'd suggest that hasValue should be contains, setValue and setRange should really be add and clearValue should be remove.

miguelportilla · 2017-05-17T12:32:24Z

@bachase The new Shard code uses lebesgue_sum. A couple of other deficiencies I found with this class- No way to clear it and no way to determine if it is empty other than comparing the first item to absent which is ugly. I suggest adding useful clear and empty functions.

The new Map type should be boost serialization access friendly as the new Shard class serializes it. Being a boost type it probably is but you may want to check. It should be protected so that a derived class can serialize.

bachase · 2017-05-17T12:44:18Z

Thanks for the feedback. I'm all for changing the interface as you suggest @nbougalis and @miguelportilla rather than the incremental changes I proposed. I'll also restore lesbegue_sum.

bachase · 2017-05-17T19:31:15Z

Get out your 🍅. RangeSet was ending up as a mostly trivial wrapper over boost::icl::interval_set, so I decided to just make it a type alias and code the non-trivial to_string and prevMissing methods as free functions. I'll admit the boost::icl has a bit of a learning curve, so I'm open to wrapping more of the interface if that is preferable.

@miguelportilla checkout https://github.com/bachase/rippled/commits/bc_shard to see the changes I made in your shard work.

miguelportilla · 2017-05-17T19:52:29Z

@bachase I like the simplification of an alias and some free functions. The shard changes look good. Using empty and clear vs the previous hacks are definitely an improvement. The "tidy" became an overhaul! Thanks and nice job! 👍

bachase · 2017-05-18T13:48:32Z

Rebased on 0.70.0-b6 and squashed with just the latest approach since the incremental change isn't particularly useful to see. Note that rs.lesbegue_sum is replaced by boost::icl::length(rs)`.

ximinez

👍 I like the rewrite.

ximinez · 2017-05-31T22:49:36Z

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

@@ -365,15 +365,16 @@ LedgerMaster::getFullValidatedRange (std::uint32_t& minVal, std::uint32_t& maxVa
    if (!maxVal)
        return false;

+    boost::optional<std::uint32_t> maybeMin;


I'm going with the idea that it's not worth changing the interface here to make minVal an optional.

If anything, I think changing this to return boost::optional<std::pair<std::uint32_t,std::uint32_t>> makes better sense.

Hm. That's a good idea. There's no reason the parameters need to be passed in to start with. Same could be said for getValidatedRange.

I'm going to punt on that for this PR. I think its worth considering, but better as a refactor/simplification of LedgerMaster.

ximinez · 2017-05-31T22:51:55Z

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

-    }
+    ScopedLockType sl(mCompleteLock);
+    if (seq > 0)
+        mCompleteLedgers.erase(range(0u, seq - 1));


Nice simplification. Can mCompleteLock be changed into a regular mutex now?

I think so, but I think the design of LedgerMaster doesn't enforce it well, so I was wary to change it.

Maybe out of scope for this change set, considering the extra testing required, but definitely worth taking a close look at.

Nice to see you adopt this change. It greatly simplifies this function and makes it easy to understand what's going on.

ximinez · 2017-05-31T22:59:45Z

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

@@ -344,14 +344,14 @@ bool
 LedgerMaster::haveLedger (std::uint32_t seq)
 {
    ScopedLockType sl (mCompleteLock);
-    return mCompleteLedgers.hasValue (seq);
+    return boost::icl::contains(mCompleteLedgers, seq);


Is it worth adding another free function to "hide" the boost::icl in this one case? Probably not.

I'd rather we don't. Rationale: it's just a function which only hides the type and complicates the codebase for anyone trying to understand. Seeing boost:: is enough for most people to say "Oh, that's Boost.. fine, I won't worry about that" unless they really need to, and in this case, they probably won't since the names are actually good enough.

nbougalis · 2017-06-01T17:15:53Z

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

@@ -344,14 +344,14 @@ bool
 LedgerMaster::haveLedger (std::uint32_t seq)
 {
    ScopedLockType sl (mCompleteLock);
-    return mCompleteLedgers.hasValue (seq);
+    return boost::icl::contains(mCompleteLedgers, seq);


I'd rather we don't. Rationale: it's just a function which only hides the type and complicates the codebase for anyone trying to understand. Seeing boost:: is enough for most people to say "Oh, that's Boost.. fine, I won't worry about that" unless they really need to, and in this case, they probably won't since the names are actually good enough.

nbougalis · 2017-06-01T17:21:24Z

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

-    }
+    ScopedLockType sl(mCompleteLock);
+    if (seq > 0)
+        mCompleteLedgers.erase(range(0u, seq - 1));


Nice to see you adopt this change. It greatly simplifies this function and makes it easy to understand what's going on.

nbougalis · 2017-06-01T20:07:44Z

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

@@ -365,15 +365,16 @@ LedgerMaster::getFullValidatedRange (std::uint32_t& minVal, std::uint32_t& maxVa
    if (!maxVal)
        return false;

+    boost::optional<std::uint32_t> maybeMin;


If anything, I think changing this to return boost::optional<std::pair<std::uint32_t,std::uint32_t>> makes better sense.

seelabs · 2017-07-12T14:27:56Z

Merged into 0.80.0-b1

bachase assigned mellery451 and ximinez May 10, 2017

bachase requested review from mellery451 and ximinez May 10, 2017 12:35

bachase mentioned this pull request May 10, 2017

Hotfix/range set get pre #2109

Closed

mellery451 suggested changes May 10, 2017

View reviewed changes

mellery451 approved these changes May 10, 2017

View reviewed changes

ximinez approved these changes May 10, 2017

View reviewed changes

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 10, 2017

bachase removed the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 11, 2017

ximinez approved these changes May 11, 2017

View reviewed changes

mellery451 approved these changes May 11, 2017

View reviewed changes

bachase force-pushed the rangeset branch from 475ca04 to 3d25fab Compare May 11, 2017 15:31

bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label May 11, 2017

nbougalis suggested changes May 17, 2017

View reviewed changes

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

bachase force-pushed the rangeset branch 2 times, most recently from 978f6b5 to 4b00186 Compare May 18, 2017 13:43

ximinez approved these changes May 31, 2017

View reviewed changes

nbougalis approved these changes Jun 1, 2017

View reviewed changes

bachase added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 2, 2017

bachase added Rebase Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. and removed Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. labels Jun 2, 2017

bachase force-pushed the rangeset branch from 4b00186 to 5d3b117 Compare June 22, 2017 16:11

bachase removed the Rebase label Jun 22, 2017

Use Boost ICL for RangeSet (RIPD-1473)

32d9e4f

bachase force-pushed the rangeset branch from 5d3b117 to 32d9e4f Compare July 10, 2017 13:33

seelabs closed this Jul 12, 2017


		BEAST_EXPECT(set.prevMissing (i) == expectedPrevMissing);
		BEAST_EXPECT(set.prevMissing(i) == expectedPrevMissing);

Tidy RangeSet #2113

Tidy RangeSet #2113

Conversation

bachase commented May 10, 2017

codecov-io commented May 10, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellery451 left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

bachase commented May 11, 2017

ximinez commented May 11, 2017

ximinez left a comment

Choose a reason for hiding this comment

mellery451 left a comment

Choose a reason for hiding this comment

bachase commented May 11, 2017

nbougalis 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

miguelportilla commented May 17, 2017 • edited Loading

bachase commented May 17, 2017

bachase commented May 17, 2017

miguelportilla commented May 17, 2017

bachase commented May 18, 2017

ximinez 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

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 Jul 12, 2017

codecov-io commented May 10, 2017 •

edited

Loading

miguelportilla commented May 17, 2017 •

edited

Loading