SHINDIG-1283 FeatureRegistry.featureMap concurrent access

[Paul Lindner, 2010-03-23 13:18:39]

A different way of dealing with
https://issues.apache.org/jira/browse/SHINDIG-1283

[zhoresh, 2010-03-23 15:14:53]

On 2010/03/23 13:18:39, Paul Lindner wrote:
> A different way of dealing with
> https://issues.apache.org/jira/browse/SHINDIG-1283

The Jira indicates the usage of ConcurrentHashMap which supposedly not required
to use synchronize. Wouldn't that be a better way then using synchronized and
copy the map for every change?

[Paul Lindner, 2010-03-23 16:02:13]

The map is constructed at startup and then never modified, so this is a
decent solution, akin to a CopyOnWrite datastructure.

On Tue, Mar 23, 2010 at 8:14 AM, <zhoresh@gmail.com> wrote:

> On 2010/03/23 13:18:39, Paul Lindner wrote:
>
>> A different way of dealing with
>> https://issues.apache.org/jira/browse/SHINDIG-1283
>>
>
> The Jira indicates the usage of ConcurrentHashMap which supposedly not
> required to use synchronize. Wouldn't that be a better way then using
> synchronized and copy the map for every change?
>
>
>
> http://codereview.appspot.com/669043/show
>

[henry.saputra, 2010-03-23 18:58:09]

http://codereview.appspot.com/669043/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
(right):

http://codereview.appspot.com/669043/diff/1/2#newcode73
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java:73:
private ImmutableMap<String, FeatureNode> featureMap;
I think this needs to be tagged as volatile to make sure all threads get the
latest version.

http://codereview.appspot.com/669043/diff/1/2#newcode455
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java:455:
protected synchronized void loadFeature(Uri parent, String xml) throws
GadgetException {
We may not need to synchronized this with volatile since the map is made
immutable.

[zhoresh, 2010-03-23 20:35:15]

On 2010/03/23 16:02:13, Paul Lindner wrote:
> The map is constructed at startup and then never modified, so this is a
> decent solution, akin to a CopyOnWrite datastructure.

In that case then it doesn't matter that much true.
But I am not sure what is the penalty in using ConcurentHashMap.

In any case I wouldn't want to use volatile as Henry suggested, since we don't
really want to serialize any read requests.


> 
> On Tue, Mar 23, 2010 at 8:14 AM, <mailto:zhoresh@gmail.com> wrote:
> 
> > On 2010/03/23 13:18:39, Paul Lindner wrote:
> >
> >> A different way of dealing with
> >> https://issues.apache.org/jira/browse/SHINDIG-1283
> >>
> >
> > The Jira indicates the usage of ConcurrentHashMap which supposedly not
> > required to use synchronize. Wouldn't that be a better way then using
> > synchronized and copy the map for every change?
> >
> >
> >
> > http://codereview.appspot.com/669043/show
> >
>

[henry.saputra, 2010-03-23 23:47:01]

Actually I thought that the approach that Paul has suggested is to allow the
read for the featureMap to make sure when new map is created, other threads
could get the latest instance of it (the proposed change to replace creating new
list to returning featureMap key set instead).

[Paul Lindner, 2010-03-24 00:09:32]

My goal was to eliminate a bunch of arraylist creations on read-only methods by
using immutable data structures internally.  As you can see it was a bit
challenging.

Instead we might want to eliminate the optional injection of

   @Named("shindig.features.default") String featureFiles

Move it to the constructor, make register() private (or protected?) and be done
with it, then no worries about concurrent access.  This is what I'd prefer, let
Singletons be Singletons.

Is anyone subclassing this, would this be a big problem?


On Mar 23, 2010, at 4:47 PM, henry.saputra@gmail.com wrote:

> Actually I thought that the approach that Paul has suggested is to allow
> the read for the featureMap to make sure when new map is created, other
> threads could get the latest instance of it (the proposed change to
> replace creating new list to returning featureMap key set instead).
> 
> 
> http://codereview.appspot.com/669043/show

[Paul Lindner, 2010-03-24 18:07:29]

patch updated by moving initialization into the constructor and some small
cleanups.

[henry.saputra, 2010-03-24 20:00:43]

LGTM

Thanks.

[zhoresh, 2010-03-24 20:52:21]

http://codereview.appspot.com/669043/diff/9001/10002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
(left):

http://codereview.appspot.com/669043/diff/9001/10002#oldcode153
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java:153:
connectDependencyGraph();
Where is this check gone to?

[Paul Lindner, 2010-03-24 21:00:08]

http://codereview.appspot.com/669043/diff/9001/10002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
(left):

http://codereview.appspot.com/669043/diff/9001/10002#oldcode153
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java:153:
connectDependencyGraph();
On 2010/03/24 20:52:22, zhoresh wrote:
> Where is this check gone to?

This moves to the main constructor.  This allows for a better separation of
concerns.  The register method generates the featureMap, and the dependency
graph generation and cache clearing move to up a level.  This is possible since
we can guarantee that register() is called only once per object.

[zhoresh, 2010-03-24 21:31:13]

LGTM

I am not so crazy about constructors that throw exceptions, but I guess it
follows the mantra of fail fast.