23
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 ...
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 ...
15
You know that those two loops are the problem (quadratic complexity - O(n2)), so, you want to decrease that somehow. Parallel.ForEach will use other cores to gain some speed, but best would be to use different structure - something 2D or 3D, to help you find the voxels faster. If 3D eats too much memory, 2D should still help (with list of voxels up/down). If ...
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 ...
12
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 ...
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
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
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
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
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 ...
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
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 ...
8
is this a correct mutex?
It is not correct. There is a requirement for std::mutex::lock (http://en.cppreference.com/w/cpp/thread/mutex/lock):
If another thread has already locked the mutex, a call to lock will
block execution until the lock is acquired.
A simple test below that uses your mutex consumes 100% CPU that means it is busy-waiting. I think ...
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;
...
7
Just a rant, someone else will do the real work, but for me it's hard to read.
durl = url;
When I see it..... what to say?
So slowly... "url" is fine, everybody understands it. "durl" could be maybe "dural" with a typo. Or "dull". There's no reason not to use the same name for a member and constructor argument. The naming in general is just terrible: "...
7
There are a number of things that I think you have completely overengineered.
Holder
This can be replaced with Optional
SingleSnapshotMap
This class exposes 4 methods:
get
put
remove
snapshot
Which is a 'light' implementation of a Map. The amount of locking though is overkill. I would go with short, sweet, and simple....
The following code takes a '...
7
Concurrency Strategy
In general, using volatile is complicated, partially because the meaning of volatile changed in Java 1.4, and also because it is hard to spot. I recommend against using it at all. Instead, you should use a more visible concurrency mechanism like synchronization, java.util.concurrent.* classes, java.util.concurrent.locks.*, or java.util....
7
Accumulator Strategy
One minor optimization can beat the single processing in a long run (\$n >= 100000000\$). Instead of update the directly AtomicSet for each non-prime number, you can update an intermediate variable with same size of each chunk of the AtomicSet and using this intermediate value to update the chunk once.
// now, mark all ...
7
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 ...
Only top voted, non community-wiki answers of a minimum length are eligible
