1
\$\begingroup\$

For a school assignment we had to implement IPC between two processes using File Maps with some specific requirements.

The relevant requirements are:

  • ComLib will send a null-terminated (C-style) string (contrary to most other circular buffers I've found online that deal only with fixed-size elements).

  • If a message doesn't fit in the end of the buffer, a dummy message should pad the end of the buffer, and the original message will be placed at the beginning (non-optimal but recommended solution).

I'd love some feedback especially on the send() and recv() functions, as they look, to me, very inefficient. This bugs me quite a bit since I'm a very performance-oriented guy. However, any feedback is appreciated!

ComLib.h

#include <Windows.h>
#include <string>
#include "SharedMemoryBuffer.h"
#include <mutex>

#define MB 1000000
typedef SharedMemoryBuffer SharedMemory;
class ComLib
{
private:
//+-+-+- Buffers -+-+-+
    SharedMemory ringBuffer;
    SharedMemory ringBufferData;

//+-+-+- Mutex -+-+-+
    HANDLE hnd_Mutex;

//+-+-+- POD members -+-+-+
    size_t ringBufferSize;
    size_t head;
    size_t tail;

//+-+-+- Private functions -+-+-+
    void UpdateRBD(size_t value);
    size_t GetTail();
    size_t GetHead();


    PVOID pRingBuffer; //?
public:
    enum TYPE{ PRODUCER, CONSUMER }type;
    enum MSG_TYPE{ NORMAL, DUMMY };
    struct Header
    {
        size_t msgId;
        size_t msgSeq;
        size_t msgLength;
    };

    // create a ComLib
    ComLib(const std::string& secret, const size_t& buffSize, TYPE type);

    // init and check status
    bool connect();
    bool isConnected();

    // returns "true" if data was sent successfully.
    // false if for any reason the data could not be sent.
    bool send(const void * msg, const size_t length);

    /*
        returns: "true" if a message was received.
                 "false" if there is nothing to read.
        "msg" is expected to have enough space, use "nextSize" to
        check this and allocate if needed, but outside ComLib.
        length should indicate the length of the data read.
        Should never return DUMMY messages.
    */
    bool recv(char * msg, size_t & length);
    /* return the length of the next message */
    size_t nextSize();

    /* disconnect and destroy all resources */
    ~ComLib();
};

ComLib.cpp

#include "ComLib.h"
#define RETURN_UNLOCK_FALSE(x) {ReleaseMutex(x); return false;}
#define RETURN_UNLOCK_TRUE(x) {ReleaseMutex(x); return true;}

#define RETURN_SAFE_FALSE RETURN_UNLOCK_FALSE(hnd_Mutex)
#define RETURN_SAFE_TRUE RETURN_UNLOCK_TRUE(hnd_Mutex)

ComLib::ComLib(const std::string& secret, const size_t& buffSize, TYPE type)
{
    //Initialize POD members
    this->type = type;
    this->head = this->tail = 0;
    this->ringBufferSize = buffSize;

    //Convert string to wide string
    std::wstring widestr = std::wstring(secret.begin(), secret.end());
    //Initialize ring buffer
    this->ringBuffer.Init(widestr.c_str(), buffSize);
    this->pRingBuffer = (PVOID)ringBuffer.GetBuffer(); //Not used

    //ringBufferData holds space for head and tail (size_t)
    //File Map is called RBD (RingBufferData)
    this->ringBufferData.Init(L"RBD", sizeof(size_t) * 2);

    //Initialize RBD and Create mutex, if we are the producer
    {
        if (this->type == PRODUCER)
        {
            this->hnd_Mutex = CreateMutex(NULL, FALSE, L"comlibmtx");
            CopyMemory(this->ringBufferData.GetBuffer(), &this->head, sizeof(size_t));
            CopyMemory(this->ringBufferData.GetBuffer() + sizeof(size_t), &this->tail, sizeof(size_t));
        }
        //If we are not the producer, open the mutex instead
        else
            this->hnd_Mutex = OpenMutex(MUTEX_ALL_ACCESS, FALSE, L"comlibmtx");
    }

}



// -------------------- [ Utility functions ] --------------------

bool MessageFits(size_t head, size_t tail, size_t bufferSize, size_t messageSize)
{
    //Check if message fits in remaining memory, between head and EOB
    //and between head and tail.
    if (bufferSize - head < messageSize)
        return false;
    if (tail > head && (tail - head) - 1 < messageSize)
        return false;
    else return true;
}

ComLib::Header CreateHeader(size_t type, size_t length /*incl null-T*/)
{
    //Create and return a header based on passed values
    ComLib::Header header;
    header.msgId = type;
    header.msgLength = length;
    header.msgSeq = 1;          //Always 1, since it is not used.
    return header;
}

void ComLib::UpdateRBD(size_t value)
{
    //Check who wants to update; producer and consumer can only update
    //their respective value (head/tail)
    if (this->type == ComLib::TYPE::PRODUCER)
    {
        CopyMemory((PVOID)ringBufferData.GetBuffer(), &value, sizeof(size_t)); //First slot is head
    }
    else
    {
        CopyMemory((PVOID)(ringBufferData.GetBuffer() + sizeof(size_t)), &value, sizeof(size_t)); //Second slot is tail
    }
}

size_t ComLib::GetTail()
{
    //Retrieve tail from RBD
    size_t r;
    CopyMemory(&r, (PVOID)(ringBufferData.GetBuffer() + sizeof(size_t)), sizeof(size_t));
    return r;
}

size_t ComLib::GetHead()
{
    //Retrieve head from RBD
    size_t r;
    CopyMemory(&r, (PVOID)ringBufferData.GetBuffer(), sizeof(size_t));
    return r;
}

bool ComLib::send(const void * msg, const size_t length)
{
    //length with header
    size_t _length = length + sizeof(ComLib::Header);

    //Acquire mutex
    WaitForSingleObject(hnd_Mutex, INFINITE);

    //Get latest tail
    this->tail = this->GetTail();

    //determine if tail is behind head in memory
    bool tailIsBehind = (tail < head);

    //Determine space left until EndOfBuffer
    size_t sizeToEOB = ringBufferSize - head;

    //Determine space left until tail
    size_t sizeToTail;
    tailIsBehind ? sizeToTail = (sizeToEOB + tail - 1) % ringBufferSize : sizeToTail = tail - head - 1;

    //If tail == head, remaining memory is size of buffer minus 1 
    //(head == tail-1 means full, in order to distinguish between empty and full)
    if (tail == head)
    {
        sizeToTail = ringBufferSize - 1;
        tailIsBehind = true;
    }


    //Check if message fits (in remaining memory and between head and tail
    //if it fits, create and send header, then send message and return true
    if (MessageFits(head, tail, ringBufferSize, _length))
    {

        auto hdr = CreateHeader(1, length);
        char* cpMsg = (char*)msg;
        char* pByte = (char*)this->pRingBuffer + head;

        //Send header and advance head
        this->ringBuffer.Send(pByte, (PVOID)&hdr, sizeof(hdr));
        pByte += sizeof(hdr);

        //send message
        this->ringBuffer.Send(pByte, cpMsg, length);
        head = (head + _length) % this->ringBufferSize;
        UpdateRBD(head);
        RETURN_SAFE_TRUE;
    }
    //if message doesnt fit and tail is in front of head, we cant pad memory;
    //return and wait for tail to advance
    else if (tail > head)
    {
        RETURN_SAFE_FALSE;
    }
    //we can not fit the message in the remaining memory. Pad and set 
    //head to 0. (Make sure tail is > 0, since that would imply that 
    //the buffer is empty, when it is in fact full. Thus we check for
    //tail < 0.
    else if (tail > 0)
    {
        // *** PAD END ***
        //if we can fit a header in the remaining memory, create a dummy
        //message. If not, just set remaining memory to '\0' and set head = 0 
        //and return false.
        if (sizeToEOB >= sizeof(Header))
        {
            auto hdr = CreateHeader(0, ringBufferSize - head);
            this->ringBuffer.Send(ringBuffer.GetBuffer() + head, (PVOID)&hdr, sizeof(hdr));
        }
        memset(ringBuffer.GetBuffer() + head, 0, ringBufferSize - head);
        head = 0;
        this->UpdateRBD(head);
        RETURN_SAFE_FALSE;
    }
    //This should be redundant. Keeping it just in case
    else if (tailIsBehind && sizeToEOB < sizeof(Header) && tail > 0)
    {
        head = 0;
        this->UpdateRBD(head);
        RETURN_SAFE_FALSE;
    }

    RETURN_SAFE_FALSE;

}

bool ComLib::recv(char * msg, size_t & length)
{
    //Acquire mutex
    WaitForSingleObject(hnd_Mutex, INFINITE);

    //Update head
    this->head = GetHead();

    //Create pointer from where we will start recieving 
    char* pByte = (char*)this->pRingBuffer + tail;

    //Determine if head is in front of tail
    bool headIsInFront = (head >= tail);

    //Determine how much memory is left in the buffer
    size_t sizeToEOB = ringBufferSize - tail;

    //Determine how much memory is left until head
    size_t sizeToHead;
    headIsInFront ? sizeToHead = head - tail : sizeToHead = ringBufferSize - tail + head;

    //Recieve header if there is space
    //for it.
    Header hdr;
    if (sizeToHead >= sizeof(Header) && sizeToEOB >= sizeof(Header))
    {
        CopyMemory(&hdr, pByte, sizeof(Header));
        pByte += sizeof(Header);
    }
    //If the remaining memory cant hold a Header, 
    //roll back to 0.
    else if (sizeToEOB < sizeof(Header))
    {
        //Make sure head is not in front (it should never be, however)
        if (!headIsInFront)
        {
            tail = 0;
            UpdateRBD(tail);
            RETURN_SAFE_FALSE;
        }
        else
        {
            RETURN_SAFE_FALSE;
        }
    }
    else RETURN_SAFE_FALSE;

    //If msgId == 1, the message is a normal message. 
    //Recieve.
    if (hdr.msgId == 1)
    {
        CopyMemory(msg, pByte, hdr.msgLength);

        tail = (tail + hdr.msgLength + sizeof(Header)) % ringBufferSize;
        UpdateRBD(tail);
        RETURN_SAFE_TRUE;
    }
    //If not, message is a dummy message (send() function
    //padded the memory. Roll back to 0.
    else
    {
        tail = 0;
        UpdateRBD(tail);
        RETURN_SAFE_FALSE;
    }

    RETURN_SAFE_FALSE;
}
\$\endgroup\$
0
\$\begingroup\$

Review of send():

Prefer char* to void* when dealing with pointers to bytes

void* refers to "literally anything" and is normally used when using dynamic typing. Since you are always passing an array of bytes, there is no dynamic typing involved, so you should use the proper static type: const char*. It will also remove a few ugly casts from your code.

Speaking of casts...

Avoid using C-style casts

(char*)msg;

C-Style casts are easy to abuse, and prevent the compiler from doing a number of important checks. For example, in your case, you accidentally casted away the constness of msg.

Use the appropriate cast for the situation, if you had forgotten the const here, the compiler would have complained:

reinterpret_cast<char*>(msg); //compile error
reinterpret_cast<const char*>(msg); //Ok!

Don't use explicit this->

For example:

this->tail = this->GetTail();
// vs
tail = GetTail();

It serves no purpose, and just makes your code harder to read. Personally, I'd recommend using a different naming convention for member variables instead. mVarName and varName_ are two popular conventions that everyone will recognize.

Use RAII to release resources

#define RETURN_UNLOCK_FALSE(x) {ReleaseMutex(x); return false;}
#define RETURN_UNLOCK_TRUE(x) {ReleaseMutex(x); return true;}
#define RETURN_SAFE_FALSE RETURN_UNLOCK_FALSE(hnd_Mutex)
#define RETURN_SAFE_TRUE RETURN_UNLOCK_TRUE(hnd_Mutex)

You have the right idea with these macros: Making sure to release the mutex before exiting the function is important. However, the C++ language has a better construct at your disposal: destructors.

class Releaser {
  HANDLE target_;
public:
  Releaser(HANDLE tgt) : target_(tgt) {}
  ~Releaser() {
    ReleaseMutex(target_);
  }
};

bool ComLib::send(const void * msg, const size_t length)
{
    //Acquire mutex
    WaitForSingleObject(hnd_Mutex, INFINITE);
    Releaser mutex_release(hnd_Mutex); // Will call ReleaseMutex when it goes out of scope.

    ...

With this, you don't have to worry about anything. The mutex WILL be released when the function exits, no matter what, even with exceptions and whatnot involved. You can also ditch your macros, and use regular old true and false for your return values.

Don't worry about performance so much

Your code obviously has a O(N) complexity, which is as good as it's going to get here. Beyond that, wait until you have identified this code as a bottlneck beofre wasting time on optimization.

\$\endgroup\$
  • \$\begingroup\$ Thank you, got some great feedback here that I'll surely use in future projects! As for send() taking void* is not my call. Most of the ComLib function prototypes were made by out teacher. I'll admit I found it somewhat odd that it took a void* but didn't really see an issue with it. What youre saying about it makes sense though! Thanks again \$\endgroup\$ – Elias Finoli Sep 20 '17 at 14:02

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.