2
\$\begingroup\$

I am trying to implement a simple raw queue in C++. This is what I have come up with

#include <queue>
#include <cstdint>
#include <array>
#include <cstring>

class simple_queue
{
private:
    static constexpr uint32_t max_msg_size = 4096;
    using char_msg = std::array<char, max_msg_size>;
    std::queue<char_msg> char_queue;

public:
    simple_queue() = default;
    ~simple_queue() = default;
    simple_queue(simple_queue&&) = default;
    simple_queue& operator=(simple_queue&&) = default;

    uint32_t write(const char* const buff, const uint32_t size) noexcept
    {
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        char_msg tmp_msg;
        std::memcpy(&tmp_msg, buff, size_to_copy);
        char_queue.push(std::move(tmp_msg));
        return size_to_copy;
    }

    uint32_t read(char* const buff, const uint32_t size) noexcept
    {
        if (char_queue.empty())
        {
            return 0;
        }
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        std::memcpy(buff, &char_queue.front(), size_to_copy);
        char_queue.pop();
        return size_to_copy;
    }
};

Note that I am trying to implement this queue just to store raw char buffers. As far as I understand making it a template does not make sense in this case.

I have played with it a bit and it seems to work as I expect.

  • What can I change with this implementation?
  • Does it make sense to use a custom allocator in here? I am allocating memory every time I write to the queue; how can I use a custom allocator to allocate some default chunk of memory when the queue is constructed?
\$\endgroup\$
2
\$\begingroup\$

You are not allocating per se when when writing to the queue. std::array is backed by a plain C-array on the stack, that gets moved.

So you only ever allocate depending on the underlying container the std::queue is based on. By default this is a std::deque.

I do not really understand the need for the std::array in the type though. Why not just use a std::string and limit the size of it to 4096? There is even a constructor that does explicitely that string (const char* s, size_t n)

Before I get to the Code there are some other things I would like to mention:

  1. why are you not useing std::copy instead of memcpy. The former works better with C++ and will in the end almost always end up as memcpy?

  2. You do not need to define the special member functions. In fact you have forgotten 2 of then, aka copy assignment and copy constructor. Why do I say forgotten? Because there is no way to tell. So If you want your queue to be move only then you should actually delete those special member functions you dont want.

  3. In your read function you never check whether the size you want to read is actually valid. Is that intendend? If so why? A std::array is not initialized so the memory in it that is not written by you will be random. You are not writing to it but you are copying it around. So you should actually take the minimum out of size and queue.front().size()

  4. std::string has a member function std::string::copy that copyies a certain amount of chars to a buffer (http://www.cplusplus.com/reference/string/string/copy/). I would suggest to use that for writing back to the buffer

That leads me to the following:

#include <string>
#include <queue>
#include <vector>

class simple_queue
{
private:
    static constexpr uint32_t max_msg_size = 4096;
    std::queue<std::string, std::vector<std::string>> char_queue;

public:  
   simple_queue(const simple_queue&) = delete;
   simple_queue& operator=(const simple_queue&) = delete;

    uint32_t write(const char* const buff, const uint32_t size) noexcept
    {
        const uint32_t size_to_copy = std::min(max_msg_size, size);
        char_queue.emplace(buff, size_to_copy);
        return size_to_copy;
    }

    uint32_t read(char* const buff, const uint32_t size) noexcept
    {
        if (char_queue.empty())
        {
            return 0;
        }
        std::string& msg = char_queue.front();
        const uint32_t size_to_copy = std::min(msg, size);
        msg.copy(buff, size_to_copy, 0);

        char_queue.pop();
        return size_to_copy;
    }
};

EDIT:

I forgot to mention, that now you should use a std::vector as backing of the queue as your are not storing a gargantuan array but rather a mall std::string.

Note that it actually ends up being the same. The std::deque based std::array implementation is backed by a linked list, so each array ends up in a node of a linked list, which is kind of similar to the separate allocation of the std::string

\$\endgroup\$
8
  • 2
    \$\begingroup\$ You should not use "EDIT tags"; they are only confusing and distracting for people who come to the question or answer for the first time. Edits happen all the time, and anyone interested can just click to see the whole post history. \$\endgroup\$ – Juho Mar 24 '19 at 12:46
  • \$\begingroup\$ Thanks, all of your points are valid. I have updated the implementation and after running benchmarks it is performing much better. However there is still something I would like to improve. With every .emplace(buff, size_to_copy) the allocator for std::string is called, I would like to get rid of allocation and have it use a pre allocated chunk of memory. I was under the impression it might be easier to do so with std::array and that's why I opted to use that instead of std::string. \$\endgroup\$ – Ali Mar 24 '19 at 17:34
  • \$\begingroup\$ @miscco BTW, I did not get why you've mentioned to use std::vector, you mean I should use a vector of chars? \$\endgroup\$ – Ali Mar 24 '19 at 17:40
  • 1
    \$\begingroup\$ @Ali std::queue is a container-adaptor. The second template argument it takes is the underlying container. Consequently, in the example i replaced the default container std::deque with std::vector \$\endgroup\$ – miscco Mar 24 '19 at 21:16
  • \$\begingroup\$ @Ali Regarding your first comment, you will most likely never get rid of all allocations, as the std array is allocated into a container node anyway. In fact it even better to allocate a potentially smaller string than always putting a gargantuan array into std::deque node \$\endgroup\$ – miscco Mar 24 '19 at 21:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.