27
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 ...
24
The reason why there is no async API for a dictionary is, that all operations on a dictionary are so fast, that there is no need for asynchronicity.
For concurrent scenarios there is the thread safe variant - the ConcurrentDictionary.
Adding an async API to these dictionaries has absolutely zero value. Rather it increases complexity and reduces performance....
21
To solve this problem, I made each Voxel aware of its neighbours, this was achieved in the world generation/height-map loading phase.
Then I was able to change the optimization algorithm to the following (found in Program.cs):
private static void DoFullOptimization(IEnumerable<Voxel> world)
{
foreach (var currentVoxel in world)
{
var ...
15
Couple of remarks:
IsSent() smells like a property, not a method. It should be more descriptive as well: what object are you calling this on? It seems to indicate Email.IsSent but that's not what you do: your description makes it sound more like EmailManager.IsDailyEmailSent.
Dispose your SmtpClient instance.
Don't set isEmailSend to true before you ...
14
Your code is not thread-safe. Each of the threads will, in parallel, be accessing both the best, and the bestQuality variables.
Your Lambda is, in essence, modifying external data from the stream, and this is an anti-pattern for streams. It has side-effects.
You should change your code to use the collect mechanism. There are a few ways to do it, but, you ...
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 ...
13
Threading Design
Your implementation has a very intrusive lock for all read and write operations, using the SemaphoreSlim with max concurrency 1.
try
{
await _semaphoreSlim.WaitAsync()// <- both read/write operations acquire single mutex
return await Task.Run(async () =>
{
return await func(_dictionary, keyValuePair);
});
}
...
12
I would expect the standard library's mutex (which is basically pthread_mutex) to already take care of this optimization behind the scenes, the same way these days I would expect the standard library's malloc to maintain its own thread-local arenas instead of taking a lock on every call.
Have you benchmarked the difference between what you wrote above and
...
12
It's pretty hard to break something that uses a global lock around everything. So this seems pretty thread-safe. But that doesn't answer the question of why you'd want to use this.
Asynchronous calls are useful for things that take a long time, particularly if you can delegate the "waiting" to some low-level event based solution (HTTP requests for example)....
11
This code in the read() method is broken:
public void read() throws InterruptedException {
synchronized (readWriteLock) {
while (numberWriters > 0 || numberWriteRequests > 0) {
readWriteLock.wait();
}
}
numberReaders++;
System.out.println("Reader #" + Thread.currentThread().getId() + " started reading.");
...
11
Your code looks sensible, with two exceptions, the unlock should only notify when the lock is unlocked.... your code currently allows for asymmetrical notifications (excessive notifications).
Consider:
public synchronized void unlock(){
if(Thread.currentThread() == lockedBy){
lockCount--;
}
if(lockCount == 0){
isLocked = false;
...
11
public class Odd implements Runnable{
private Monitor sharedObject;
public Odd(Monitor monitor){
this.sharedObject = monitor;
}
You implemented the class as a Runnable - great first step. You are injecting the shared Monitor in the constructor, which is good; the member variable should be final though -- you aren't intending to change it after ...
11
Naming
There is one 'u' in ConcuurentCircularBuffer that wants to be a 'r' --> ConcurrentCircularBuffer
There is no need to abbreviate name like size (sz --> size)
I would call last to something more descriptive like lastIndex.
Code Style
sz, buffer and lockObject should be read-only.
There is no need to skip the first array element, just start with 0.
...
11
Some suggestions:
The trailing slash in the mkdir command is redundant.
$(…) is preferred over backticks for command substitution.
Why use seq in one command? They both do the same loop, so you might as well use {1..100} in both places.
Semicolons are unnecessary in the vast majority of cases. Simply use a newline to achieve the same separation between ...
10
Normally, you don't catch Exceptions to throw them again, it doesn't make sense, let them rise up, don't catch them here, I don't think it is your intention to stop the exceptions here, so don't.
Let the Exceptions bubble up.
If you are checking in a database to see if it is sent already you could save to the database with a dateSent column or something ...
10
There are several thread safety issues :
if size(), remove() and removeAll() need to wait(), they will throw an IllegalMonitorStateException since they do not hold the lock on this at the time they call wait();
for that same reason, they may see stale values for isUpdating
if size() would be interrupted while waiting (supposing that gets fixed) its return ...
10
In addition to @JanDotNet's answer:
You can use "less strict" lock in your code, specifically ReaderWriterLock or ReaderWriterLockSlim. As I see from your code, you can execute Read in parallel threads but cannot execute Put and Read in parallel. These classes are exactly for this case and will improve overall performance.
10
The code for apple and linux cases is identical (except one case). Consider
#if defined(__APPLE__) || defined(__linux__)
#define POSIX
#endif
and consolidate the identical blocks under #ifdef POSIX. Or consistently do
#ifdef __WIN32__
....
#else
....
#endif
as you did near the end of forp.
Determining a number of processors by parsing an output of an ...
9
My experience with variants on the java.util.concurrent.locks.* classes suggests that their sweet spot in terms of performance is when the work to be done is relatively substantial compared to the lock overhead time. Remember, when you use Reentrant or ReadWrite locks, that there is a a call at both the beginning and end of the locked block.
As a ...
9
A few things that caught my eye:
So called dispose pattern only makes sense if you implement a finalizer on your base class or on one of its descendants. If you do not finalize anything - don't bother with passing bool flag, that is always true. YAGNI.
Always use the highest level tool available to you, especially if you are not comfortable with multi-...
9
First I'm not a fan of recycling IDs. Too much confusion can happen. For example Thread 1 adds a Dog Image and gets ID 1. Thread 2 calls ReleaseAllImages and then adds a Cat image and gets ID 1. Now Thread 1 think it has a Dog image but instead returns back a Cat image. There are times we need to recycle ID but only you know the business requirements if ...
9
If you modify the AsyncDictionary while enumerating its keys/values it throws InvalidOperationException (if the backing dictionary is a Dictionary).
var numbers = new AsyncDictionary<int, int>();
foreach(var number in Enumerable.Range(1, 1000))
{
await numbers.AddAsync(number, number);
}
foreach(var number in await numbers.GetKeysAsync())
{
...
8
@William Morris mentioned the spinning on a lock issue (busy wait). This is a real problem that you should address.
Empty block at the end of while is non obvious to read.
You should comment on it so that people don't think it is a mistake:
while(this->interlock == 1 || InterlockedCompareExchange(&this->interlock, 1, 0) == 1);
// I would do:
...
8
1. Thread safety
From a concurrency stand-point, I don't see any issue with the code: reading is guarded by the readLock, all the updates are guarded by the writeLock, and you're using thread-safe BlockingDeques for the values.
However, you're returning the actual BlockingDeque<Peer> from the Cache in getPeers(), so you're not mandating going through ...
8
You can make your code much simpler if you use a LinkedList<T> instead of an array. It has also a much better performance when inserting and removing items in this case which are both O(1) operations:
public class ConcurrentCircularBuffer<T> : ICircularBuffer<T>
{
private readonly LinkedList<T> _buffer;
private int ...
7
Things you did well on:
Overall, this looks like a pretty decent little bit of code. It looks like some research went into this
I like the comments. Everywhere I was confused on something, there was a comment to clarify. You also didn't go overboard with them!
Things you could improve on:
Preprocessor:
Group all of your #defines at the top of your ...
7
http://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#max-java.util.Comparator-
Returns the maximum element of this stream according to the provided Comparator. This is a special case of a reduction.
Furthermore, there's a handy static method on Comparator to lift a regular function to a comparator:
http://docs.oracle.com/javase/8/docs/...
7
you should be using a couple of using statements in your code, then you can get rid of the try/finally statement completely because the closing/disposing of the connection and command are done automatically.
so this:
string connectionString = ConfigurationManager.ConnectionStrings["connectionString"].ConnectionString;
bool isSent = false;
...
Only top voted, non community-wiki answers of a minimum length are eligible
