Thread-safety and data structures fixes

Modification in this branch:

1) Some flags were not thread safe, especially some mutual shared flag where modification visibility must be ensured. 
One case was PeerDiscovery. I was thinking to use a volatile, but an AtomicBoolean suits better here.
The problem here is that with a normal memory-zone the visibility of a modification made by one thread could not be seen by another thread, due to "compiling" optimizations. Volatile turn off those optimizations, but you lose in performance. An Atomic* object ensures atomicity with a non-blocking lock, gaining in performance.
In other simpler cases as PeerDiscoveryMonitorThread I used volatile.

2) On EthereumImpl, the PeerData findPeer(PeerData peerData) it has a Spin-lock on peer discovery 
that needs to be removed as it can lead poor performance and poor concurrency. Resolved with a 
BlockingQueue. The findPeer now it´s much simpler and no more Thread.sleep()

   while ((peer = peers.take()) != null) { // it blocks until a peer is available.
            
            if (peer.isOnline() && !peer.equals(peerData)){

                logger.info("Found peer: {}", peer.toString());

                if (listener != null)
                    listener.trace(String.format("Found online peer: [ %s ]", peer.toString()));

                return peer;
            }
        }

3) WorldManager had a race-condition thread bug into the lazy initialization. This can be solved in many
way, using a volatile reference for the instance object and a syncronized getter, but it is not very 
performant. A better solution is the Initialization-on-demand holder idiom
https://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom, a JVM pattern that allow a lazy 
highly efficient thread-safe "singleton" instantiation, without synchronization overhead.

4) Still on WorldManager private final List<PeerData> peers = new CopyOnWriteArrayList<PeerData>(); 
is not the most suitable collection. It is VERY memory expensive. Use a ConcurrentQueue here, not a 
CopyOnWrite. That collection should only be used with near-unmodificable collections as a 
gui-windows-event listeners. Even more, there´s a performante problem in the add Peers

    public void addPeers(List<PeerData> newPeers) {
        for (PeerData peer : newPeers) {
            if (this.peers.indexOf(peer) == -1) {

                this.peers.add(peer);
                if (peerDiscovery.isStarted())
                    peerDiscovery.addNewPeerData(peer);
            }
        }
    }

This routine has O(n^2) complexity. IndexOf has O(n) complexity furthermore calling the resource expensive .equals().
Using a queue is not the correct collection to use. We have 2 ways. Or we use a Map (O(1) constant access, with the ip as a key) Or even better, we use a Set. A set is a collection that ensures uniqueness of its objects. So you dont´ have to check if a peer is already inside the peer collection. It is the set that ensure that for us (and funnily an HashSet is implemented in the JVM internally with an hashmap...). We should use Set whenever we want to ensure uniqueness. And *NOT* to remove the objects after. One example is in the PeersMessage.

And now some considerations:

5) We should use much more final references and promote immutabiity. This helps a lot for thread-safety.

6) All the loggers should have a static and final reference. There is no point in instantiating multiple
loggers for the same class, it's just a waste of memory and initialization resources.

7) About style: verbosity is the worst enemy of Java. Please I would put no-logic methods (as getter and setters) at the end of the classes and first the notable logicfully-meaningfully methods. This is improve redeability, I would use more fluent interfaces too...but it is a kind of personal style.

8) LoggerFactory.getLogger("peerdiscovery"); in a class as EthereumPeerTasterHandler makes me tricky to figure out which class is logging. We must use the class name. LoggerFactory.getLogger(EthereumPeerTasterHandler.class);

9) We should promote higher level collection classes. Uses "Collection<>" rather than List<>. Let the implementor of the classes to decide the best collection strategy.


7 - Interesting, I understand your points, and actually I agree that´s much
easier to add a design pattern rather than removing it. I don´t want to
follow the risk to overengineer the code, that can come quite easy.

9 - Definitely I will follow the leader guidelines! :-)

Reverting back the logs...



On Wed, Aug 27, 2014 at 6:05 AM, romanman <notifications@github.com> wrote:

> In src/main/java/org/ethereum/core/Blockchain.java:
>
> > @@ -52,46 +52,30 @@
> >   */
> >  public class Blockchain {
> >
> > -	private static Logger logger = LoggerFactory.getLogger("blockchain");
> > -	private static Logger stateLogger = LoggerFactory.getLogger("state");
> > +	private static final Logger logger = LoggerFactory.getLogger(Blockchain.class);
>
> Fix it as I mentioned , and all places that is a look like.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/ethereum/ethereumj/pull/100/files#r16756550>.
>