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.


Roman the take () is a blocking operation... it won't cycle if the queue is
empty.
On 27 Aug 2014 11:21, "romanman" <notifications@github.com> wrote:

> In src/main/java/org/ethereum/facade/EthereumImpl.java:
>
> > @@ -103,16 +100,6 @@ public void addListener(EthereumListener listener) {
> >          WorldManager.getInstance().addListener(listener);
> >      }
> >
> > -
> > -    private void sleep10Milli(){
>
> question: don't you think if you don't sleep in a thread it will be too
> greedy on a cpu time
> when it doesn't need it ? really asking it because on old operational
> systems I do remember
> having troubles if the check loop doesn't sleep for a while , I don't know
> for 100% if in the modern
> systems it wasn't prevented in any way.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/ethereum/ethereumj/pull/100/files#r16765172>.
>