Allocation Optimizations by kevinkreiser · Pull Request #29 · vthiery/cpp-statsd-client

kevinkreiser · 2021-06-15T02:15:06Z

In a previous PR I noticed that we instantiated a char[] only to copy it into a string and then potentially copy that string further on down the chain. In that previous PR I changed the char[] to a fixed size string but still the string was being allocated on ever call to send and again potentially copied.

This PR moves that buffer to a member variable, to which we write our messages and from which we read to send bytes over the socket. When in batching mode I've also made some changes to remove as many re-allocations as possible (by reserving the batch size for any new batches that are made). I've changed the send interface of the UDPSender to use iterators so that we don't copy any bytes from the StatsdClient to the UDPSender.

One strange quirk that I noticed a while back that we dont handle is when a person tries to send a stat that is larger than our buffer. snprintf tells us when this happens but we previously didnt do anything about it. i've added a unit test for that case but annoyingly since the UDPSender owns the error message it was somewhat un-tidy to allow the StatsdClient to set this message.

…im it, persist the buffer and copy a portion of it when needed

…ore reallocations

kevinkreiser · 2021-06-15T02:15:42Z

                                  const std::string& prefix,
                                  const uint64_t batchsize) noexcept
-    : m_prefix(prefix), m_sender(new UDPSender{host, port, batchsize}) {
+    : m_prefix(prefix), m_sender(new UDPSender{host, port, batchsize}), m_buffer(256, '\0') {


256 seems big enough to me but maybe sometime in the future we could make it configurable

kevinkreiser · 2021-06-15T02:17:36Z

    }

-    // Prepare the buffer and include the sampling rate if it's not 1.f
-    std::string buffer(256, '\0');


now we dont have to make a new string every time we send

kevinkreiser · 2021-06-15T02:20:34Z

-        } else {
-            m_batchingMessageQueue.rbegin()->append("\n").append(message);
+            m_batchingMessageQueue.emplace_back();
+            m_batchingMessageQueue.back().reserve(m_batchsize + 256);


when we need a new batch started, we now reserve enough bytes that we will not need to reallocate it again.

We could make this function slightly clearer with something like

inline void UDPSender::send(const std::string& message) noexcept { m_errorMessage.clear(); // If batching is on, accumulate messages in the queue if (m_batching) { queueMessage(message); return; } sendToDaemon(message); } inline void UDPSender::queueMessage(const std::string& message) noexcept { std::unique_lock<std::mutex> batchingLock(m_batchingMutex); // Either we don't have a place to batch our message or we exceeded the batch size, so make a new batch if (m_batchingMessageQueue.empty() || m_batchingMessageQueue.back().length() > m_batchsize) { m_batchingMessageQueue.emplace_back(); m_batchingMessageQueue.back().reserve(m_batchsize + 256); } // When there is already a batch open we need a separator when its not empty else if (!m_batchingMessageQueue.back().empty()) { m_batchingMessageQueue.back().push_back('\n'); } // Add the new message to the batch m_batchingMessageQueue.back().append(message); }

kevinkreiser · 2021-06-15T02:21:49Z

            close(m_socket);
            m_socket = k_invalidFd;
-            m_errorMessage = "getaddrinfo failed: error=" + std::to_string(ret) + ", msg=" + gai_strerror(ret);
+            m_errorMessage = "getaddrinfo failed: err=" + std::to_string(ret) + ", msg=" + gai_strerror(ret);


i tried to make all the error message look as much like each other as possible. its still not perfect but slightly better i guess

kevinkreiser · 2021-06-15T02:22:38Z

-    if (!client.errorMessage().empty()) {
-        std::cerr << client.errorMessage() << std::endl;
-        throw std::runtime_error(client.errorMessage());
+void throwOnError(const StatsdClient& client, bool expectEmpty = true, const std::string& extraMessage = "") {


i realized i wanted to test when i expected there not to be an error AND when i expected there to be an error. so i extended this helper to allow for testing both possiblities

kevinkreiser · 2021-06-15T02:23:12Z

-        std::cerr << "Should not be able to connect to ridiculous ip" << std::endl;
-        throw std::runtime_error("Should not be able to connect to ridiculous ip");
-    }
+    throwOnError(client, false, "Should not be able to resolve a ridiculous ip");


so now testing for when we expect an error is way less verbose.

vthiery · 2021-06-15T14:37:37Z

Thanks for the work and all the comments! I'll review it asap

kevinkreiser · 2021-06-15T14:39:04Z

@vthiery in the mean time i will get the conflict resolved! thank you so much!

kevinkreiser · 2021-06-15T15:00:44Z

@vthiery the conflicts should be resolved now, let me know what you think!

kevinkreiser · 2021-06-21T13:54:02Z

@vthiery is there anything i should do here to make this easier for review or are you just trying to find the time for it? let me know if there is anything else i should do here!

vthiery · 2021-06-21T15:32:21Z

@vthiery is there anything i should do here to make this easier for review or are you just trying to find the time for it? let me know if there is anything else i should do here!

Sorry for the delay. There is nothing left to do on your side, I just need to find some time to review it properly. I'll try to do it tomorrow.

kevinkreiser · 2021-06-22T02:08:35Z

Well I started implementing the other PR for protecting against sending the wrong stats types and I was adding support for tags and I started to question what would be the best way to support tags. This lead me down a rabbit hole of what is the best way to format a string. I was pretty shocked at the results, but checkout this gist:

https://gist.github.com/kevinkreiser/1a7c9a7cc803a51e79f76a5e0e53730f#file-string_format-cpp-L17-L20

Based on my testing it seems using string operations directly is faster than printf-like functions, which I think makes decent sense. So I'm going to refactor this PR to do that as well.

kevinkreiser · 2021-06-22T02:22:51Z

-                                   value,
-                                   type.c_str(),
-                                   frequency);
+    // Format the stat message


it was faster to use string operations than snprintf, i benchmarked it here: https://gist.github.com/kevinkreiser/1a7c9a7cc803a51e79f76a5e0e53730f#file-string_format-cpp

the other benefit is that now we dont have to worry if the string we are format is larger than the buffer we have. when for some reason there is a really large stat message, the std::string will allocate more space to fit it. in practice this should be very unlikely.

kevinkreiser · 2021-06-22T02:26:10Z

Ok cool I've updated the code to use string operations which simplifies this PR, makes the code faster, and makes the code easier to update in my next PR 😄

vthiery

Looks good! Only a couple of minor details to fix

vthiery · 2021-06-22T07:07:04Z

-        } else {
-            m_batchingMessageQueue.rbegin()->append("\n").append(message);
+            m_batchingMessageQueue.emplace_back();
+            m_batchingMessageQueue.back().reserve(m_batchsize + 256);


We could make this function slightly clearer with something like

inline void UDPSender::send(const std::string& message) noexcept { m_errorMessage.clear(); // If batching is on, accumulate messages in the queue if (m_batching) { queueMessage(message); return; } sendToDaemon(message); } inline void UDPSender::queueMessage(const std::string& message) noexcept { std::unique_lock<std::mutex> batchingLock(m_batchingMutex); // Either we don't have a place to batch our message or we exceeded the batch size, so make a new batch if (m_batchingMessageQueue.empty() || m_batchingMessageQueue.back().length() > m_batchsize) { m_batchingMessageQueue.emplace_back(); m_batchingMessageQueue.back().reserve(m_batchsize + 256); } // When there is already a batch open we need a separator when its not empty else if (!m_batchingMessageQueue.back().empty()) { m_batchingMessageQueue.back().push_back('\n'); } // Add the new message to the batch m_batchingMessageQueue.back().append(message); }

Co-authored-by: Vincent Thiery <vjmthiery@gmail.com>

kevinkreiser · 2021-06-22T12:37:23Z

ok lint should be good now, there were spaces in the extra lines between the string formating code blocks. thanks again @vthiery!

vthiery · 2021-06-22T12:39:27Z

Thanks for the contribution @kevinkreiser !

kevinkreiser added 2 commits June 14, 2021 20:52

rather than continuously allocate buffer and then deallocate it to tr…

720635a

…im it, persist the buffer and copy a portion of it when needed

add unit tests to confirm behavior when message is too large. avoid m…

2019353

…ore reallocations

kevinkreiser commented Jun 15, 2021

View reviewed changes

Comment thread include/cpp-statsd-client/StatsdClient.hpp Outdated

kevinkreiser commented Jun 15, 2021

View reviewed changes

Comment thread include/cpp-statsd-client/StatsdClient.hpp Outdated

kevinkreiser commented Jun 15, 2021

View reviewed changes

Comment thread include/cpp-statsd-client/StatsdClient.hpp Outdated

kevinkreiser commented Jun 15, 2021

View reviewed changes

resolve conflicts

6bd4d23

actually using string operations is both faster and less complicated

0225ed9

kevinkreiser commented Jun 22, 2021

View reviewed changes

revert unneeded change

eeb6459

vthiery reviewed Jun 22, 2021

View reviewed changes

kevinkreiser and others added 3 commits June 22, 2021 08:31

whitespace for clarity

3e223ce

Co-authored-by: Vincent Thiery <vjmthiery@gmail.com>

break out queuing into its own function

12cf74a

whitespace in the whitespace haha

aaf98df

vthiery approved these changes Jun 22, 2021

View reviewed changes

vthiery merged commit 61af069 into vthiery:master Jun 22, 2021

jakila mentioned this pull request Jan 28, 2022

If the client is thread safe? #12

Open

Conversation

kevinkreiser commented Jun 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vthiery commented Jun 15, 2021

Uh oh!

kevinkreiser commented Jun 15, 2021

Uh oh!

kevinkreiser commented Jun 15, 2021

Uh oh!

kevinkreiser commented Jun 21, 2021

Uh oh!

vthiery commented Jun 21, 2021

Uh oh!

kevinkreiser commented Jun 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinkreiser commented Jun 22, 2021

Uh oh!

vthiery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinkreiser commented Jun 22, 2021

Uh oh!

vthiery commented Jun 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants