Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy RangeSet #2113

Closed
wants to merge 1 commit into from
Closed

Tidy RangeSet #2113

wants to merge 1 commit into from

Conversation

@bachase
Copy link
Contributor

bachase commented May 10, 2017

@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

This comment has been minimized.

Copy link

codecov-io commented May 10, 2017

Codecov Report

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

Impacted file tree graph

@@             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.

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

This comment has been minimized.

Copy link
@mellery451

mellery451 May 10, 2017

Contributor

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 👍

This comment has been minimized.

Copy link
@bachase

bachase May 10, 2017

Author Contributor

Good suggestion. I will add.

This comment has been minimized.

Copy link
@bachase

bachase May 10, 2017

Author Contributor

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.

Copy link
Contributor

mellery451 left a comment

LGTM

Copy link
Contributor

ximinez left a comment

Removed code is code that can't break! 👍

@ximinez ximinez added the Passed label May 10, 2017
@bachase bachase removed the Passed label May 11, 2017
@bachase

This comment has been minimized.

Copy link
Contributor Author

bachase commented May 11, 2017

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

This comment has been minimized.

Copy link
Contributor

ximinez commented May 11, 2017

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

Copy link
Contributor

ximinez left a comment

Nice abstraction. 👍

Copy link
Contributor

mellery451 left a comment

👍 I like it

@bachase bachase force-pushed the bachase:rangeset branch from 475ca04 to 3d25fab May 11, 2017
@bachase bachase added the Passed label May 11, 2017
@bachase

This comment has been minimized.

Copy link
Contributor Author

bachase commented May 11, 2017

Squash and passed, thanks!

Copy link
Member

nbougalis left a comment

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis May 17, 2017

Member

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

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);

This comment has been minimized.

Copy link
@nbougalis

nbougalis May 17, 2017

Member

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis May 17, 2017

Member

std::cout in a unit test?

This comment has been minimized.

Copy link
@bachase

bachase May 17, 2017

Author Contributor

D'oh. Forgot to tidy that.


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

This comment has been minimized.

Copy link
@nbougalis

nbougalis May 17, 2017

Member

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.


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

This comment has been minimized.

Copy link
@nbougalis

nbougalis May 17, 2017

Member

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.

@nbougalis nbougalis removed the Passed label May 17, 2017
@miguelportilla

This comment has been minimized.

Copy link
Member

miguelportilla commented May 17, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

bachase commented May 17, 2017

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

This comment has been minimized.

Copy link
Contributor Author

bachase commented May 17, 2017

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

This comment has been minimized.

Copy link
Member

miguelportilla commented May 17, 2017

@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 bachase force-pushed the bachase:rangeset branch 2 times, most recently from 978f6b5 to 4b00186 May 18, 2017
@bachase

This comment has been minimized.

Copy link
Contributor Author

bachase commented May 18, 2017

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)`.

Copy link
Contributor

ximinez left a comment

👍 I like the rewrite.

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

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

This comment has been minimized.

Copy link
@ximinez

ximinez May 31, 2017

Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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

This comment has been minimized.

Copy link
@ximinez

ximinez Jun 1, 2017

Contributor

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.

This comment has been minimized.

Copy link
@bachase

bachase Jun 2, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@ximinez

ximinez May 31, 2017

Contributor

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

This comment has been minimized.

Copy link
@bachase

bachase Jun 1, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@ximinez

ximinez Jun 1, 2017

Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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

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

This comment has been minimized.

Copy link
@ximinez

ximinez May 31, 2017

Contributor

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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.

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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

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

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

This comment has been minimized.

Copy link
@nbougalis

nbougalis Jun 1, 2017

Member

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

@bachase bachase added the Passed label Jun 2, 2017
@bachase bachase added Rebase Passed and removed Passed labels Jun 2, 2017
@bachase bachase force-pushed the bachase:rangeset branch from 4b00186 to 5d3b117 Jun 22, 2017
@bachase bachase removed the Rebase label Jun 22, 2017
@bachase bachase force-pushed the bachase:rangeset branch from 5d3b117 to 32d9e4f Jul 10, 2017
@seelabs

This comment has been minimized.

Copy link
Contributor

seelabs commented Jul 12, 2017

Merged into 0.80.0-b1

@seelabs seelabs closed this Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.