-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add transaction rate setting to perfclient #1071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/azp run |
master@7385 aka 20200417.2 vs master ewma over 50 builds from 6937 to 7385 |
@lynshi you need to format your code, that can be done with:
|
@@ -379,6 +391,12 @@ namespace client | |||
size_t read; | |||
size_t written; | |||
|
|||
if (options.transactions_per_s > 0) { | |||
write_delay_us = 1000000 / options.transactions_per_s; | |||
connection->set_tcp_nodelay(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set TCP_NODELAY
in all cases, outside of this check. Even without a target transaction rate, this should make the performance tests more predictable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In throughput tests, I don't expect it will produce a measurable effect, we are writing heavily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I have some minor suggestions to simplify the code, and please run clang-format
.
samples/perf_client/perf_client.h
Outdated
@@ -431,13 +449,31 @@ namespace client | |||
size_t& written, | |||
const std::shared_ptr<RpcTlsClient>& connection) | |||
{ | |||
if (write_delay_us > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, we can remove time_elapsed_between_writes_ns
entirely:
while (std::chrono::high_resolution_clock::now() - last_write_time < write_delay)
{
continue;
}
Then we can remove the if (write_delay > 0)
checks here and below - this while loop will always break on the first iteration, and writing last_write_time
all the time is harmless.
samples/perf_client/perf_client.h
Outdated
@@ -289,6 +297,10 @@ namespace client | |||
timing::ResponseTimes response_times; | |||
timing::CommitPoint last_response_commit = {0, 0}; | |||
|
|||
std::chrono::high_resolution_clock::time_point last_write_time; | |||
int64_t time_elapsed_between_writes_us; | |||
int64_t write_delay_us = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be more strongly typed: std::chrono::nanoseconds write_delay = 0;
(I'd prefer nanoseconds rather than microseconds so we keep some precision as we approach 100k tx/s)
Thanks for the suggestions @eddyashton; I've implemented them. However, I still only set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
/azp run |
The
--transaction-rate
flag allows one to specify the transaction rate (tx/s) of the client. If a transaction rate is specified, the client computes the interval between writes needed to maintain this rate, and waits this amount between writes.