ethereum/ethereumj — pull request #100 by davassi (2014-08-26 15:20:36+00:00)
Title: Thread-safety and data structures fixes

Modification in this branch:

1) Some flags were not thread safe, especially some mutual shared flag where 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 could not be seen
by another thread, due to "compile" 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 peers = new CopyOnWriteArrayList();
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 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:

3) We should use much more final references and promote immutabiity. This helps a lot for thread-safety.

4) 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.

5) 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.

6) 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);

7) We should promote higher level collection classes. Uses "Collection<>" rather than List<>. Let the implementor of the classes to decide the best collection strategy.