hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Rawson <ryano...@gmail.com>
Subject Re: Review Request: HBASE-2578
Date Wed, 26 May 2010 03:54:16 GMT
Thanks for that salient comment. Perhaps someone can give us the right
pattern for no lock Singleton initialization.

On May 25, 2010 8:49 PM, "Paul Cowan" <cowan@aconex.com> wrote:
> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message