On 26/05/2010 1:15 PM, Daniel Ploeg wrote:
this is a lot of synchronization for what ends up being on the fast path of every query, perhaps there is a non synchronized manner in which we can accomplish this? the use of volatile might help herethanks Ryan. I think I might use AtomicReference instead - can simplify even further this way. I'll upload another patch a bit later.
Sorry, no real stake in this review but just a curious third-party: is there any need for even the AtomicReference?
Making 'delegate' volatile would completely remove the need for the synchronization in getDelegate(), and the lock on injectEdge(). The only exception is that without synchronization, the null-check-and-default in getDelegate() would be subject to race conditions. Given the desire to reduce synchronization, and the fact that delegate is assigned to default at construction time, I'd humbly suggest that the "if (delegate == null)" be removed from getDelegate(), and instead change injectEdge() to reject null values (or call reset() if you want to keep the effective existing semantics that injectEdge(null) returns to a DefaultEnvironmentEdge).
Then the ReentrantLock can be removed, no synchronization is required, and one volatile variable is all that's needed.
(Incidentally, I'm not sure what the ReentrantLock is actually FOR -- reference assignment is atomic in Java, so it's not protecting against any race conditions; all it could be useful for is a happens-before relationship with the get, but the two are using different locks -- the synchronized get does not have a happens-before relationship with the unlock. Either both should be synchronized, or both use the lock -- as it is, the lock buys you nothing. Mind you with a simple volatile then the happens-before relationship is correct anyway).
Cheers, Paul
