24

Prefer Reference over Pointer Since the producer and consumer must have a buffer you should pass it by reference (rather than pointer). This also makes sure there is no confusion over ownership of the buffer (the owner of a pointer is responsible for deleting it). By using a RAW pointer you can not tell the owner but by using a reference you are explicitly ...


14

I have a few comments about your code: Using std::lock_guard is great to handle mutexes since it automatically unlocks the acquired mutex when leaving the scope. That's a real great tool. You should really use it everywhere you can. Using it consistently will make sure that you can't forget to unlock any mutex. Moreover, it makes sure that mutexes are ...


11

Style Standard C# naming convention for methods is PascalCase. For static and instance members there are more variants around but often they are prefixed with _ and/or area also PascalCase so they can be easily distinguished from local variables and parameters. Following standard naming conventions makes your code look more familiar to other C# developers. ...


10

static int avail = 0; ... while (avail == buffSize) { }; ... avail++; You can't just access the same field from multiple threads at the same time and assume it will work correctly. As already pointed out by ChrisWue, incrementing a value is not an atomic operation, and you could fix that by using Interlocked.Increment() instead. Another problem is ...


10

First of all it would be good to use the standard Java Conventions, i.e. method names start with a lower case letter. i.e. StartconsumerProducer shold either be startconsumerProducer or simply run. Also handle InterruptedException correctly. For details have a look at the book Java Concurrency in Practice In your case it does not make sense to catch the ...


10

Design The problem with your implementation of pop() is that it can not be implemented in an exception safe manner (for any type of T). This is why the standard implementation implements this as two different methods top() and pop(). The top() function simply returns the value while pop() does not return the value but simply removes the value from head. So ...


7

Issues: Failed to check the return value of all these system calls: stack->storage = malloc(sizeof(void*) * stack->capacity); pthread_mutex_init(&stack->mutex, NULL); pthread_cond_init(&stack->empty_condition_variable, NULL); pthread_cond_init(&stack->full_condition_variable, NULL); pthread_mutex_lock(&stack->mutex); ...


7

The implementation clearly has a race condition: Let's assume Count == _boundedCapacity - 1 Now have 2 producers call Insert simultaneously Both simultaneously check that the current size is less than the upper bound and move beyond the while loop Both insert an item. Now you've exceeded the bounded capacity by 1 While it might be ok to be able to exceed ...


6

#include <condition_variable> std::condition_variable cv; #include <iostream> #include <random> #include <mutex> std::mutex mtx; #include <thread> Don't mix includes and code, makes it easy to miss those globals you're defining in the middle. Keep the headers by themselves sorted in some order or grouped logically/by ...


6

Your consumer thread locks out the producer for extended periods of time. void ConsumeData() { while (true) { std::unique_lock<std::mutex> uniqueLock(mutex); conditionVariable.wait(uniqueLock, [] {return !queue.empty(); }); while (!queue.empty()) { // DO WORK queue.pop(); } uniqueLock.unlock(); } } I ...


5

You implemented your producers and consumers as Runnable -- that's good. You are also, judging from your sample output, using a thread naming mechanism that makes it easier to identify what's going on -- that's very good. Not bad. Buckle up. There are a couple immediate problems with your producer logic. First, your producers are generating identical ...


5

Did you know that Task.Result blocks? Your code isn't asynchronous at all - and it can't be unless DoRead is. Effectively your code is starting a new task and then blocking while it completes - there's no benefit to starting the task at all! As a small aside, Task.Run is easier than using Task.Factory.StartNew. If I were you, I'd just remove the Task.


5

When calling the function malloc(): The returned type is void*, so it can be assigned to any pointer, making the 'cast' totally unneeded. It just clutters the code, making it more difficult to understand, debug, maintain. The expression sizeof(char) is defined in the standard at 1. Multiplying anything by 1 has no effect, so the expression just clutters the ...


5

Dead Lock Bug If the queue is empty and the consumer thread is waiting in the condition variable. If you Queue goes out of scope the main thread will enter the detructor and perform join(). At this point you are locked. As both threads are waiting for the other. Use of condition can be simplified. while(queue.empty()) { cv.wait(lock); }...


5

You have comments in the definition of RingBuffer on the value limits for some members, but there is nothing in the code that enforces that or checks to see if those limits are exceeded. Why does TryRead take its parameter by const int&? The reference is unnecessary. Just pass in the int. Why is MAX_SIZE a macro? It should be a constexpr (if your ...


5

In addition to what has already been said I would add that what happens if s is zero? You might want to throw an exception in the constructor if that happens probably because any call to count of room would fail otherwise. you can definitely improve const correctness at least in a couple of places: in explicit PCQueue(size_t s) : you can make s const. ...


5

This is sort of an extended comment on Martin York's reply. When you're doing any sort of parallel processing, I advise against re-designing pop so it requires two operations to actually remove an item from the queue, like: T val = queue.top(); queue.pop(); With sufficient care, this can work for a single-producer/single-consumer situation, but has the ...


4

First of all: your code is not bad; I agree with Wayne Conrad. I have a minor remark about the connect statement. You are currently using Qt 4.8, so this is fine as it is. However with Qt 5 a new signal/slot syntax was introduced. Your question about threading: Don't be afraid of QThread, they're just threads too. The problem with QThread is only this: a ...


4

As a matter of being thread-safe, you should replace this: private void TimerOnElapsed(object sender, ElapsedEventArgs e) { if (BatchAvailable != null) BatchAvailable(sender , e); } with this: private void TimerOnElapsed(object sender, ElapsedEventArgs e) { var batchAvailable = this.BatchAvailable; ...


4

Producer and Consumer You properly synchronize on the sharedList. Both Producer and Consumer fail to use the proper wait() idiom to guard against spurious wake up. Producer waits until the list is empty : while(!sharedList.isEmpty()) sharedList.wait(); Consumer waits until the list is not empty : while(sharedList.isEmpty()) sharedList.wait(); ...


4

Just a few nit-picky things: You are using C++, so it would be wise to split up your code into a header and source file. Showing your include statements would be helpful. I see no documentation on your methods. Unless it's something very straightforward (like a programming contest entry), methods in your public code should generally have some form of ...


4

Below are my suggestions grouped by class and the code including these suggestions at the end. CircularBuffer I would advice using an implementation of BlockingQueue (for your case ArrayBlockingQueue seems appropriate). Nevertheless, below are my suggestions to improve this class : Use a specific variable for the number of available elements You know the ...


4

Extraneous line I quickly scanned your stack code and it looked correct to me. However, I did spot an extraneous line in concurrent_stack_top(). You have this line that was probably copied from concurrent_stack_pop(): pthread_cond_signal(&stack->full_condition_variable); Since concurrent_stack_top() doesn't remove anything from the stack, you ...


4

Buffer<T> This class seems fine for the most part. Some small issues: The while(true) loops in insert(T) and remove() only ever run one iteration. Those loops can be replaced by their body. add calls m_buffer.push_back(x), which might not compile for types without a copy constructor. Try m_buffer.push_back(std::move(x)) instead: That will move if ...


3

Queue queue = new Queue(10); servicePool = new ServiceThreadPool(10,queue); requestPool = new RequestThreadPool(1000,queue); You should replace those numbers with constants. Reading that I'm not sure -- is the ServiceThreadPool the same size as the queue? Or is that a coincidence? With constants, it would be obvious. An ...


3

I don't really understand how you want to use this so I'm just going to add a couple of comments for you to ponder. Naming It's more common to abbreviate Asynchronous to Async not Asynch when used in identifers: AsynchSimpleProducer<T> -> AsyncSimpleProducer<T>. NewItem isn't a great name for an event, events generally need a verb to make ...


3

Robust code does not assume the first char in buf is not '\0'. This is a pedantic point, but user IO is a hacker paradise - best to code defensively. Instead of: size_t str_len; while (fgets(buf, sizeof(buf), fp) != NULL) { str_len = strlen(buf); if(buf[str_len - 1] != '\n') printf("Didn't read whole line!\n"); else { buf[str_len - 1] = '\0';...


3

You are trying to align each ref object to a cache line by padding the object to fill an entire cache line. However your refs vector is embedded as a member in the multiqueue class hence it will start at an offset relative to the beginning of the object which is probably either aligned on a 4 or 8 byte boundary. So your refs vector might start in the middle ...


3

Definitely not optimal as you are holding locks longer than you need too. boost::unique_lock<boost::mutex> lk(_frameCacheMutex); /// Here create the data for cache... // You are creating data here. // But you should not be interacting with the cache here. // So why are you holding a lock? // You should only hold a lock while you ...


3

I just have some stylistic things for a start: Since your custom types are in PascalCase, your function names should instead be in camelCase or in snake_case. This isn't a requirement, but it makes it easier to distinguish between them. The naming convention used for the private members looks like a form of Hungarian Notation, which is quite disliked in ...


Only top voted, non community-wiki answers of a minimum length are eligible