msg = disp.QueryFreeMessage();
if(msg)
....
T* QueryFreeMessage() { return &queue[iWriteMarker]; }
if(msg) will always be true.
Dispatch can fail, but the caller doesn't know
Also, in this case, the caller has already used QueryFreeMessage to get a pointer to a queue-slot, and has written data into that slot, even though the queue is full (overwriting not-yet-consumed data).
You probably want to make QueryFreeMessage actually return false to solve this, and change the error inside Dispatch into an assertion failure, because it shouldn't ever happen if the client is using the class correctly.
GetMessage increments the read cursor, which lets the other thread know that it's safe to override that slot... but if the write thread does reuse that slot before HandleMessage is called, then you'll have data that's being leaked (never actually getting consumed), and other data that gets consumed twice. To solve that, you'd have to only increment the read cursor after the data has been consumed.
Or, Instead of returning T*'s to the user, return a T by value, which has been copied before the cursor has been incremented.
Ah yes - indeed. Thanks for the keen observations. The first one I was aware of and the solution I put in place was to silently drop the overflowed messages and dump a notification to the console. It isn't really my intention to have the final code do anything remarkable when QueryFreeMessage() fails (as it would after the amendments you propose), because I don't intend to implement any queue resizing or wait contingency anyway.
The broader picture I'm actually somewhat more concerned about is that in terms of communicating between the input and update (render) threads, at the end of the day I don't think there's a way to make it lock-free. Or rather I'm failing to grasp how to effectively go about synchronization.
Consider the below two calls. The following example is simplistic, but it hilights the problem rather effectively: how can I guarantee that GetSize(), when called from an arbitrary thread, will actually return the correct values? In fact, while the SetSize()-GetSize() example is trivial, for any application that has several threads that depend on the window's current width and height, this seems like a potential cluster-bork.
In other words - I think I've been far too concerned about making the updates non-concurrent, but what is instead much more worrisome is how the data can be reliably read.
//public, called by any thread
Window::SetSize(int w, int h)
{
//directly calls DoSetSize() if called by consumer thread, otherwise posts the message to queue
DispatchWindowMessage(this, w, h);
}
//called by any thread
Window::GetSize(int &w, int &h)
{
//simple read, assumes both values are up-to-date
w = width;
h = height;
}
//only accessible to the consumer thread
Window::DoSetSize(int w, int h)
{
//can set these without a lock, but what if GetSize() is called at the same time from another thread?
width = w;
height = h;
}
A simple solution would be to leave this for the application to worry about, but I I'm more interested in how this kind of synchronization is achieved in something more critical, like an operating system.
For what its worth, if anybody knows this software:
http://www.e-onsoftware.com/products/vue/
I'm the co-author of the opengl rendering together with christophe riccio (http://www.g-truc.net/).
So basically, the opengl preview viewports of Vue has a GUI-thread message producer; and Render-thread message consumer queue system, based on a dual std::vector that swaps during consumption (each frame); and each message "push" takes the lock (boost mutex + lock) and each consumption also before swapping the queue. It just so happens that in common situations, more than 200 000 messages are pushed by second, and it is by no way a bottleneck.
I just mean to say, if you are afraid of 512 locks per frame... there is a serious problem in your "don't do too much premature optimization" thinking process, that needs fixing.
I agree that its total fun to attempt to write a lock free queue, but, if it was production code, frankly not worth the risk; and plainly misplaced focus time.
Now just about the filter thing, one day, just to see, I made a filter to avoid useless duplication of messages, it worked but was slower than the raw, dumb queue. I idon't say its absolutely going to be the same in your case; just try... but in my case being clever was being slower.
Thanks for the heads up! I was actually more surprised about the number of messages I was receiving for the amount of windows I had on my GUI. The truth is that I simply didn't have a good grasp of how much communication was going on in my code. I hardly went so far as to optimize this - the filter in this case was rather more useful for debugging purposes :)