Improve trx_generator http client #1699
Conversation
Also add better error handling for ec
| req_.set(http::field::connection, "close"); | ||
| req_.body() = std::move(request_body); | ||
| req_.keep_alive(false); |
There was a problem hiding this comment.
Why these changes? I would think that keep_alive is better?
There was a problem hiding this comment.
It was an attempt to get nodeos to not hold on to the connection as long.
Note that nodeos has this: https://github.com/AntelopeIO/leap/blob/main/plugins/http_plugin/include/eosio/http_plugin/beast_http_session.hpp#L114
There was a problem hiding this comment.
In this case, wouldn't the transaction generator send a bunch of requests to the node, and benefit from the connection not dropping after each request?
There was a problem hiding this comment.
It would if the trx_generator reused a socket. It creates a new connection for each request. We really need to fix that because on my machine I run out of ports.
There was a problem hiding this comment.
Thanks. For posterity, maybe add a comment that we disable keep_alive because the trx_generator doesn't reuse sockets, but this needs to be changed, and then keep_alive should be re-enabled.
| @@ -503,7 +503,8 @@ class beast_http_session : public detail::abstract_conn, | |||
| try { | |||
| // Send a shutdown signal | |||
| beast::error_code ec; | |||
| socket_.shutdown(Socket::shutdown_send, ec); | |||
| socket_.shutdown(Socket::shutdown_both, ec); | |||
| socket_.close(ec); | |||
There was a problem hiding this comment.
Can you explain this change a little more. I understand what it's doing but I don't follow if the PR is suggesting this is what improves performance, and if so, why.
There was a problem hiding this comment.
This change less about performance and more about leaving a socket in timed wait on client abandoning a socket. The eof is used for error conditions, so makes sense to shutdown both.
There was a problem hiding this comment.
This change got rid of TIME_WAIT states?
There was a problem hiding this comment.
No, the keep_alive change helped with the TIME_WAIT. This was for when it was already in a bad state.
There was a problem hiding this comment.
Not sure I understand what the purpose of this entire do_eof() function is. Letting the socket's shared_ptr fall out of scope is going to close it anyways.
There was a problem hiding this comment.
I'll try experimenting with just removing it.
There was a problem hiding this comment.
In 4f4f5d3 removed do_eof, resulted in read-only-trx-parallel-test failing do to Failure when receiving data from the peer which looks like it is http error 56. This test does a lot of http requests to nodeos.
It does look like the do_eof is needed. The example boost code also has the do_eof.
@spoonincode I will revert 4f4f5d3 unless you can find something wrong in my removal of it.
There was a problem hiding this comment.
However, with the removal of do_eof, this branch reported a 15501 TPS.
There was a problem hiding this comment.
Seems like we might have a problem somewhere; I'm still not understanding why it's needed. But yeah might as well put it back for now.
This reverts commit 4f4f5d3.
When error conditions (eof) have the
http_pluginshutdown_bothon socket. Also callclose()for good measure. I actually tried this explicit shutdown in the destructor but it seemed to make no difference.Modify the
trx_generatorto better handle errors from client and also specify no keep alive.This improved performance as can be seen by comparing:
This branch: https://github.com/AntelopeIO/leap/actions/runs/6356928532 15001 TPS
With main: https://github.com/AntelopeIO/leap/actions/runs/6356929999 14001 TPS