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

Grow TxQ expected size quickly, shrink slowly (RIPD-1534): #2235

Closed
wants to merge 1 commit into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Sep 27, 2017

  • Stores recent history of "good" ledgers. Uses the maximum as the
    expected ledger size. When a large value drops off, use a 90%
    backoff to go down to to the new maximum.
  • If consensus is unhealthy, wipe the history in addition to the current
    clamping.
  • Include .md doc files in xcode and VS projects

Open question: Does this need to be behind an amendment? I don't think so because it only affects transaction queueing, not transaction processing, just as 0.70.2 didn't need an amendment.

return *iter;
// 90% backoff to decrease
// down to the actual max.
return (txnsExpected_ * 9 + *iter) / 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm reading this as 90% of txnsExpected_ plus 10% of "recent tx count max"...so the result will always be slightly less thantxnsExpected_...correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. It allows the limit to come down gradually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find the comment somewhat confusing. Can we brainstorm on some better wording?

@codecov-io
Copy link

codecov-io commented Oct 2, 2017

Codecov Report

Merging #2235 into develop will increase coverage by <.01%.
The diff coverage is 92.3%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2235      +/-   ##
===========================================
+ Coverage    70.11%   70.11%   +<.01%     
===========================================
  Files          689      689              
  Lines        50800    50810      +10     
===========================================
+ Hits         35617    35626       +9     
- Misses       15183    15184       +1
Impacted Files Coverage Δ
src/ripple/app/misc/TxQ.h 96.77% <100%> (+0.1%) ⬆️
src/ripple/app/misc/NetworkOPs.cpp 61.74% <100%> (ø) ⬆️
src/ripple/app/misc/impl/TxQ.cpp 95.85% <90.9%> (-0.1%) ⬇️
src/ripple/app/misc/SHAMapStoreImp.cpp 78.92% <0%> (-1.03%) ⬇️
src/ripple/protocol/impl/Serializer.cpp 70.3% <0%> (+0.34%) ⬆️
src/ripple/protocol/impl/STVar.cpp 88.31% <0%> (+2.59%) ⬆️
src/ripple/core/Stoppable.h 100% <0%> (+6.66%) ⬆️

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 cafe18c...de3805f. Read the comment docs.

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Tentative approve from me; I've left a couple of small comments for your consideration. No changes needed really; it's at your discretion.

return *iter;
// 90% backoff to decrease
// down to the actual max.
return (txnsExpected_ * 9 + *iter) / 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the comment somewhat confusing. Can we brainstorm on some better wording?

std::min(feeLevels.size(), *maximumTxnCount_) :
feeLevels.size();
std::min(next, *maximumTxnCount_) :
next;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be overly complex with no good reason. Consider:

txnsExpected_ = std::min(next, maximumTxnCount_.value_or(next));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much better.

}
else if (feeLevels.size() > txnsExpected_ ||
feeLevels.size() > targetTxnCount_)
{
recentTxnCounts_.emplace_back(feeLevels.size());
while (recentTxnCounts_.size() > setup.ledgersInQueue)
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit of code suggests that a std::deque may not be the right choice here. Have we considered boost::circular_buffer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had not, but that's a great idea.

* Stores recent history of "good" ledgers. Uses the maximum as the
  expected ledger size. When a large value drops off, use a 90%
  backoff to go down to to the new maximum.
* If consensus is unhealthy, wipe the history in addition to the current
  clamping.
* Include .md doc files in xcode and VS projects
@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Nov 11, 2017
@bachase
Copy link
Collaborator

bachase commented Nov 30, 2017

Merged as c11e186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants