Tune for higher transaction processing. #2294

mtrippled · 2017-12-10T01:12:10Z

No description provided.

ripplelabs-jenkins · 2017-12-10T01:38:10Z

Jenkins Build Summary

Built at 20180117 - 17:11:31

Test Results

Build Type	Result	Status
clang.debug.unity	975 cases, 0 failed, t: 386s	PASS ✅
coverage	975 cases, 0 failed, t: 612s	PASS ✅
gcc.debug.unity	975 cases, 0 failed, t: 436s	PASS ✅
clang.debug.nounity	973 cases, 0 failed, t: 709s	PASS ✅
clang.release.unity	974 cases, 0 failed, t: 470s	PASS ✅
gcc.debug.nounity	973 cases, 0 failed, t: 756s	PASS ✅
gcc.release.unity	974 cases, 0 failed, t: 512s	PASS ✅

codecov-io · 2017-12-10T02:02:55Z

Codecov Report

Merging #2294 into develop will decrease coverage by 0.05%.
The diff coverage is 59.45%.

@@            Coverage Diff             @@
##           develop   #2294      +/-   ##
==========================================
- Coverage    70.96%   70.9%   -0.06%     
==========================================
  Files          691     691              
  Lines        51663   51409     -254     
==========================================
- Hits         36661   36452     -209     
+ Misses       15002   14957      -45

Impacted Files	Coverage Δ
src/ripple/core/JobQueue.h	`100% <ø> (ø)`	⬆️
src/ripple/json/json_value.h	`93.93% <ø> (+0.18%)`	⬆️
src/ripple/overlay/Overlay.h	`78.94% <ø> (ø)`	⬆️
src/ripple/overlay/impl/PeerImp.cpp	`0% <0%> (ø)`	⬆️
src/ripple/app/misc/HashRouter.h	`86.48% <0%> (-13.52%)`	⬇️
src/ripple/app/misc/HashRouter.cpp	`87.03% <0%> (-12.97%)`	⬇️
src/ripple/core/impl/JobQueue.cpp	`86.38% <100%> (-0.19%)`	⬇️
src/ripple/app/misc/NetworkOPs.cpp	`62.77% <100%> (ø)`	⬆️
src/ripple/resource/impl/Fees.cpp	`100% <100%> (ø)`	⬆️
src/ripple/app/main/Application.cpp	`60.61% <100%> (-0.35%)`	⬇️
... and 114 more

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 090d813...e24689b. Read the comment docs.

scottschurr

Changes look good to me. I left one thought for a potential change. I think it's a good idea, but it's not required.

I don't have any opinion on the constant value changes. I'll defer to @ximinez on those.

Thanks for chasing this down.

scottschurr · 2017-12-11T20:21:26Z

src/ripple/overlay/impl/PeerImp.cpp

@@ -45,6 +45,9 @@ using namespace std::chrono_literals;

 namespace ripple {

+// The maximum number of transactions to have in the job queue.
+int const max_transactions = 1000;


Since this constant is only used one place in the file, consider reducing its scope to just where it is needed. Here's an example of what I have in mind: scottschurr@99f3bfc

BTW, I like providing a name for the magic number. Good work with that.

The constant value changes decrease the cost by 10x for peers to send transactions to one another, while leaving the relative for all other activities the same.

sublimator · 2017-12-11T21:16:54Z

soon ...

scottschurr · 2017-12-11T21:28:05Z

👍

ximinez

I left a couple of comments identifying unused Charge values.

I also suggest updating the commit message to something like

Tune for higher transaction processing:

* Decrease the relative cost for `feeLightPeer`, which is charged when a transaction is received, by increasing all the other charges.

If not, at least remove the ..

Otherwise, looks good, so I'll approve once those changes are made.

ximinez · 2017-12-14T00:15:23Z

src/ripple/resource/impl/Fees.cpp

+Charge const feeHighBurdenRPC     ( 3000, "heavy RPC"              );
+
+Charge const feeLightPeer         (    1, "trivial peer request"   );
+Charge const feeLowBurdenPeer     (   20, "simple peer request"    );


feeLowBurdenPeer is unused. May as well remove it.

ximinez · 2017-12-14T00:18:42Z

src/ripple/resource/impl/Fees.cpp

+Charge const feeReferenceRPC      (   20, "reference RPC"          );
+Charge const feeExceptionRPC      (  100, "exceptioned RPC"        );
+Charge const feeLightRPC          (   50, "light RPC"              ); // DAVID: Check the cost
+Charge const feeLowBurdenRPC      (  200, "low RPC"                );


feeLightRPC and feeLowBurdenRPC are unused. May as well remove them.

ximinez · 2017-12-14T00:20:33Z

src/ripple/resource/impl/Fees.cpp

+
+Charge const feeNewTrustedNote    (  100, "trusted note"           );
+Charge const feeNewValidTx        (  100, "valid tx"               );
+Charge const feeSatisfiedRequest  (  100, "needed data"            );


feeNewTrustedNote, feeNewValidTx, and feeSatisfiedRequest are unused. May as well remove them.

ximinez

👍 pending CI.

JoelKatz · 2017-12-14T21:32:02Z

We found an issue where transactions were being dispatched each time they were received from a peer. This caused transactions to be dropped due to the inflated backlog and servers to be dropped due to the inflated counts of extraneous transactions received from peers. We changed this to suppress dispatching a transaction received from a peer if that transactions was dispatched due to peer receipt within the past ten seconds.

mtrippled · 2017-12-14T22:09:03Z

@JoelKatz changes LGTM for the new suppressor
👍 plus it syncs to the network. So pending jenkins I say OK.

JoelKatz

👍 Let's try to get this to the cluster soon to improve the performance there.

mtrippled · 2017-12-15T18:14:33Z

@JoelKatz No transactions were dropped. However, each transaction was being distributed through the network multiple times to each peer. The duplicates caused congestion control limits to kick in, causing some peers to sever connections with other peers. The only peers who experienced issues with this were the public-facing client handlers and not the validators. No transactions were dropped.

sublimator · 2017-12-15T18:17:19Z

~50

mtrippled · 2017-12-20T01:18:55Z

@ximinez @scottschurr @nbougalis @JoelKatz I've reopened this PR because it has been modified since we last visited it. This PR prior to commit e24689b was put into 0.80.2. The latest commit (e24689b) mainly adds counters to track some behaviors surrounding desync events.

scottschurr

In general, nice looking changes. Especially if they help us identify performance issues.

I left comments that you may want to address in a few places.

scottschurr · 2017-12-20T18:46:33Z

src/ripple/json/impl/json_value.cpp

+    std::string str(std::to_string(value));
+    value_.string_ = valueAllocator ()->duplicateStringValue (
+        str.c_str (), str.size() );
+}


I'm a bit nervous about introducing these constructors. Once these constructors exist, if someone constructs a Value with an int32_t, the Value has intValue. But if they construct with an int64_t, the Value has stringValue. I think this will potentially lead to bugs where folks don't know what size of integer they are handling (auto anyone?) and sometimes produce strings instead of integers.

The change is convenient for your particular use case. But I think it has the potential to make bugs easier to write.

If you want to keep this change you'll need to convince me that it is safe for the code base in general, not just the code you're introducing today.

Without those constructors, creating json from 64bit input won't compile (at least on gcc):

/home/mtravis/projects/rippled/src/ripple/app/misc/NetworkOPs.cpp:2392:34: error: conversion from ‘uint64_t {aka long unsigned int}’ to ‘Json::Value’ is ambiguous
info[jss::jq_trans_overflow] = app_.overlay().getJqTransOverflow();

Referring to NetworkOPs.cpp:2392:
info[jss::jq_trans_overflow] = app_.overlay().getJqTransOverflow();

No existing 64bit integers rendered as Json will be harmed, because none exist other than what I'm putting there.

In other places where we want to store 64-bit integers in JSON, we use to_string to make it clear that JSON treats them differently.

I agree with @scottschurr that doing this by default has the potential for confusing semantics going forward.

How about this? ximinez@a51519b

I've reverted the json 64bit constructor to go with the team's consensus that 64bit integers are not to be handled automatically but instead make the coder remember to make them strings. The only json change I kept is initializing the map_ object and protecting against double-free on destruction.

scottschurr · 2017-12-20T18:59:10Z

src/ripple/overlay/impl/OverlayImpl.h

+    }
+
+    void
+    incPeerDisconnect()


Should be marked override.

scottschurr · 2017-12-20T18:59:31Z

src/ripple/overlay/impl/OverlayImpl.h

+    }
+
+    void
+    incPeerDisconnectCharges()


Should be marked override.

ximinez · 2017-12-18T21:09:22Z

src/ripple/app/misc/HashRouter.cpp

@@ -71,6 +71,18 @@ bool HashRouter::addSuppressionPeer (uint256 const& key, PeerShortID peer, int&
    return result.second;
 }

+bool HashRouter::shouldProcess (uint256 const& key, PeerShortID peer, int& flags,
+    Stopwatch::time_point now, std::chrono::seconds interval)


Why make the caller responsible for now and interval? shouldRelay (which, yeah, I wrote) uses the clock from within the suppressionMap_.

Fixed, though using the clock from suppressionMap_ fails unit test so am using the C++ clock.

ximinez · 2017-12-18T21:11:32Z

src/ripple/app/misc/HashRouter.cpp

+    auto& s = result.first;
+    s.addPeer (peer);
+    flags = s.getFlags ();
+    return s.shouldProcess (now, interval);


This could use some unit tests.

ximinez · 2017-12-21T00:08:46Z

src/ripple/json/impl/json_value.cpp

+    std::string str(std::to_string(value));
+    value_.string_ = valueAllocator ()->duplicateStringValue (
+        str.c_str (), str.size() );
+}


How about this? ximinez@a51519b

scottschurr · 2018-01-12T01:46:33Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -120,7 +120,7 @@ class NetworkOPsImp final
    {
        struct Counters
        {
-            std::uint64_t transitions = 0;
+            std::uint32_t transitions = 0;


Did you intend to change this back to uint32_t? If so, then I think you could skip the uses of std::to_string() below. But I don't believe that's your intent.

Yes, I intended to revert and I did revert the conversion for transitions. And to_string() was already in place for the dur member, so that's unchanged.

Thanks Mark. It looks like the added std::to_string() calls deal with your removal of the 64-bit JSON interface. Sorry I didn't catch that before.

scottschurr

👍 I'm good with these changes. I'll let @ximinez speak for the changes he suggested in HashRouter.cpp.

scottschurr · 2018-01-12T17:25:33Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -120,7 +120,7 @@ class NetworkOPsImp final
    {
        struct Counters
        {
-            std::uint64_t transitions = 0;
+            std::uint32_t transitions = 0;


Thanks Mark. It looks like the added std::to_string() calls deal with your removal of the 64-bit JSON interface. Sorry I didn't catch that before.

ximinez

The test can work with the suppression map clock. I left a comment describing how.

ximinez · 2018-01-16T21:53:04Z

src/test/app/HashRouter_test.cpp

+
+        BEAST_EXPECT(router.shouldProcess(key, peer, flags, 1s));
+        BEAST_EXPECT(! router.shouldProcess(key, peer, flags, 1s));
+        std::this_thread::sleep_for(2s);


Replace this line with

++stopwatch; ++stopwatch;

Then change HashRouter::shouldProcess to use supressionMap_.clock().now() instead of steady_clock. Or cherry-pick ximinez@d1f584b

The test should now pass, and as a bonus, the tests aren't unnecessarily slowed down.

@ximinez Fixed: it's almost as if stopwatch is something that can be started and stopped explicitly.

Exactly. Now that I think about it, stopwatch is a terrible name. It's more of a manualClock.

sublimator · 2018-01-17T16:12:17Z

Nah, that manualCloxk sounds terrible

ximinez

Good to go. 👍

sublimator · 2018-01-17T16:14:22Z

no fun

Do not process a transaction received from a peer if it has been processed within the past ten seconds. Increase the number of transaction handlers that can be in flight in the job queue and decrease the relative cost for peers to share transaction and ledger data. Additionally, make better use of resources by adjusting the number of threads we initialize, by reverting commit 68b8ffd. Performance counter modifications: * Create and display counters to track: 1) Pending transaction limit overruns. 2) Total peer disconnections. 3) Peers disconnections due to resource consumption. Avoid a potential double-free in Json library.

scottschurr · 2018-01-19T18:18:53Z

Incorporated into 0.90.0-b4 as 76ad06e.

mtrippled requested review from ximinez and scottschurr December 10, 2017 01:12

scottschurr assigned ximinez and scottschurr Dec 11, 2017

scottschurr approved these changes Dec 11, 2017

View reviewed changes

ximinez requested changes Dec 14, 2017

View reviewed changes

mtrippled force-pushed the transactions branch 2 times, most recently from 2695b2f to 565ab5a Compare December 14, 2017 00:51

ximinez approved these changes Dec 14, 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 Dec 14, 2017

mtrippled force-pushed the transactions branch 2 times, most recently from 355981e to c73fc8e Compare December 14, 2017 21:52

nbougalis approved these changes Dec 14, 2017

View reviewed changes

JoelKatz approved these changes Dec 14, 2017

View reviewed changes

mtrippled force-pushed the transactions branch from c73fc8e to b986447 Compare December 14, 2017 23:37

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

scottschurr reviewed Dec 20, 2017

View reviewed changes

ximinez reviewed Dec 21, 2017

View reviewed changes

nbougalis mentioned this pull request Jan 10, 2018

Proposed 0.90.0-b3 #2328

Merged

scottschurr reviewed Jan 12, 2018

View reviewed changes

scottschurr approved these changes Jan 12, 2018

View reviewed changes

mtrippled force-pushed the transactions branch 2 times, most recently from 77c5f7a to 43457be Compare January 13, 2018 05:38

ximinez requested changes Jan 16, 2018

View reviewed changes

mtrippled force-pushed the transactions branch from 43457be to fbd5493 Compare January 17, 2018 03:21

mtrippled added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 17, 2018

ximinez approved these changes Jan 17, 2018

View reviewed changes

ximinez added the Rebase label Jan 17, 2018

mtrippled force-pushed the transactions branch from fbd5493 to f0a4f90 Compare January 17, 2018 16:35

scottschurr removed the Rebase label Jan 17, 2018

scottschurr closed this Jan 19, 2018

Tune for higher transaction processing. #2294

Tune for higher transaction processing. #2294

Conversation

mtrippled commented Dec 10, 2017

ripplelabs-jenkins commented Dec 10, 2017 • edited Loading

Jenkins Build Summary

Test Results

codecov-io commented Dec 10, 2017 • edited Loading

Codecov Report

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

sublimator commented Dec 11, 2017 via email

scottschurr commented Dec 11, 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

ximinez left a comment

Choose a reason for hiding this comment

JoelKatz commented Dec 14, 2017

mtrippled commented Dec 14, 2017

JoelKatz left a comment

Choose a reason for hiding this comment

mtrippled commented Dec 15, 2017

sublimator commented Dec 15, 2017 via email

mtrippled commented Dec 20, 2017

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

bachase Dec 20, 2017 • 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

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

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

sublimator commented Jan 17, 2018 via email

ximinez left a comment

Choose a reason for hiding this comment

sublimator commented Jan 17, 2018 via email

scottschurr commented Jan 19, 2018

ripplelabs-jenkins commented Dec 10, 2017 •

edited

Loading

codecov-io commented Dec 10, 2017 •

edited

Loading

bachase Dec 20, 2017 •

edited

Loading