hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Cowan <co...@aconex.com>
Subject Re: Review Request: HBASE-2578
Date Wed, 26 May 2010 03:48:05 GMT
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 here
>
> thanks 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

Mime
View raw message