river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patricia Shanahan <p...@acm.org>
Subject Re: MarshalledServiceItem
Date Tue, 08 Feb 2011 16:51:18 GMT
There are some negatives to using CHM compared to wrapping a HashMap in 
a synchronized map. It is easy to write methods for compound operations 
such as removing a key-value pair if, and only if, some other key is 
present.

Patricia


On 2/8/2011 8:26 AM, Dan Creswell wrote:
> No friction, simplest thing that could possibly work is the only thing
> running through my head.
>
> I'm happy to see CHM used if people deem it appropriate - as is so often the
> case, I'm not writing the code so my opinion should only carry so much
> weight.
>
> On 8 February 2011 15:12, Gregg Wonderly<gregg@wonderly.org>  wrote:
>
>> On 2/8/2011 9:00 AM, Dan Creswell wrote:
>>
>>> It's all about expected usage patterns.
>>>
>>> Locking in this case will be per instance and the likelihood of contention
>>> is low given no one's going to call add a lot or indeed toString a lot in
>>> a
>>> short space of time.
>>>
>>> So given:
>>>
>>> Define how concurrency is handled then implement
>>>
>>> And assuming it's in line with the above contention model I'd say
>>> synchronized would be adequate.
>>>
>>
>> I believe this is the case.  But, I also know that it can come back to
>> bite.
>>
>> CHM solves it without synchronized, and that, I believe is a better choice
>> because it eliminates the problem.  Any additional methods or usage patterns
>> that fall out of the life cycle of this object will not have any new or
>> different behavior or synchronization requirements.
>>
>> Is there an attribute of CHM that you find unattractive Dan?  I'm not sure
>> I understand the friction I am sensing.
>>
>> Gregg
>>
>>
>>   On 8 February 2011 14:54, Gregg Wonderly<gregg@wonderly.org>   wrote:
>>>
>>>   On 2/7/2011 5:28 PM, Peter Firmstone wrote:
>>>>
>>>>   Do we really need a concurrent version of this class? How often will
>>>>> Entry
>>>>> classes change?
>>>>>
>>>>>
>>>> My choice is always to consider concurrency first, unless there is some
>>>> element of performance that this impacts.  The simple fact is that there
>>>> is
>>>> a toString() that traverses the contents.  So, you either must
>>>> synchronize
>>>> all access to the contents, or use a CHM which solves the problem without
>>>> any future concurrency surprises.
>>>>
>>>> I'm really serious about this, because of how I've encountered tons of
>>>> "synchronized" blocks in river which introduce huge latency in
>>>> "processing"
>>>> because they cause everyone to wait in line for stuff which very often is
>>>> quite independent.
>>>>
>>>> Yes, this structure is mostly one time mutated.  But, its design exposes
>>>> a
>>>> continuous mutation capability.  Thus, any "read" of the contents has to
>>>> take that into account.
>>>>
>>>> Gregg Wonderly
>>>>
>>>>
>>>>   Gregg Wonderly wrote:
>>>>
>>>>>
>>>>>   Yes definitely the case. CHM just makes it a lot easier to not worry
>>>>>> about map
>>>>>> and set data structures in terms of concurrent access of mutating
data.
>>>>>> Set
>>>>>> and HashSet are useful, but not always in concurrent mutating data
>>>>>> management.
>>>>>>
>>>>>> I still prefer Vector over ArrayList just because it eliminates
>>>>>> iteration
>>>>>> problems that alway require the same lines of code to resolve.
>>>>>>
>>>>>> Gregg
>>>>>>
>>>>>> Sent from my iPhone
>>>>>>
>>>>>> On Feb 7, 2011, at 2:08 PM, Dan Creswell<dan.creswell@gmail.com>
>>>>>>   wrote:
>>>>>>
>>>>>>   All true, still need to document the concurrent contract for this
>>>>>> class
>>>>>>
>>>>>>> and
>>>>>>> implement accordingly.
>>>>>>>
>>>>>>> i.e. We can say "toString" is thread safe etc and then use
>>>>>>> ConcurrentHashMap
>>>>>>> and friends.
>>>>>>>
>>>>>>> But we gotta do it right....
>>>>>>>
>>>>>>> On 7 February 2011 19:57, Gregg Wonderly<gregg@wonderly.org>
  wrote:
>>>>>>>
>>>>>>>   On 2/7/2011 5:24 AM, Tom Hobbs wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>   Sorry, I've got my nit-picking hat on...
>>>>>>>>
>>>>>>>>>
>>>>>>>>> The null check in the equals method isn't needed because
you get a
>>>>>>>>> free one by using "instanceof".
>>>>>>>>>
>>>>>>>>> I know you've warned that the class is not thread-safe,
but people
>>>>>>>>> often forget that doing a sysout of an object calls toString().
I'd
>>>>>>>>> still be tempted to whack some protection into toString
to stop it
>>>>>>>>> blowing up when people start printing lots of debug messages.
>>>>>>>>>
>>>>>>>>> Other than that it looks good to me.
>>>>>>>>>
>>>>>>>>>   I'd suggest that it would be easy enough to just use
>>>>>>>>>
>>>>>>>> ConcurrentHashMap
>>>>>>>> instead of HashSet if we are going to move to Java 5 anyway.
Then,
>>>>>>>> you
>>>>>>>> don't have to worry about concurrency issues with toString()
etc.
>>>>>>>>
>>>>>>>> Gregg Wonderly
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Feb 7, 2011 at 10:56 AM, Peter Firmstone<jini@zeus.net.au>
>>>>>>>>
>>>>>>>>   wrote:
>>>>>>>>>
>>>>>>>>>   Dan Creswell wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>   Solid - probably ought to be a constructor that
doesn't add
>>>>>>>>>> defaults
>>>>>>>>>>
>>>>>>>>>>> too.....and maybe a note about thread safety
on add.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>   The finished article.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> /*
>>>>>>>>>> * Licensed to the Apache Software Foundation (ASF)
under one
>>>>>>>>>> * or more contributor license agreements. See the
NOTICE file
>>>>>>>>>> * distributed with this work for additional information
>>>>>>>>>> * regarding copyright ownership. The ASF licenses
this file
>>>>>>>>>> * to you under the Apache License, Version 2.0 (the
>>>>>>>>>> * "License"); you may not use this file except in
compliance
>>>>>>>>>> * with the License. You may obtain a copy of the
License at
>>>>>>>>>> *
>>>>>>>>>> * http://www.apache.org/licenses/LICENSE-2.0
>>>>>>>>>> *
>>>>>>>>>> * Unless required by applicable law or agreed to
in writing,
>>>>>>>>>> software
>>>>>>>>>> * distributed under the License is distributed on
an "AS IS" BASIS,
>>>>>>>>>> * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
express or
>>>>>>>>>> implied.
>>>>>>>>>> * See the License for the specific language governing
permissions
>>>>>>>>>> and
>>>>>>>>>> * limitations under the License.
>>>>>>>>>> */
>>>>>>>>>>
>>>>>>>>>> package org.apache.river.api.lookup;
>>>>>>>>>>
>>>>>>>>>> import java.util.HashSet;
>>>>>>>>>> import java.util.Iterator;
>>>>>>>>>> import java.util.Set;
>>>>>>>>>> import net.jini.lookup.entry.Address;
>>>>>>>>>> import net.jini.lookup.entry.Comment;
>>>>>>>>>> import net.jini.lookup.entry.Location;
>>>>>>>>>> import net.jini.lookup.entry.Name;
>>>>>>>>>> import net.jini.lookup.entry.ServiceInfo;
>>>>>>>>>> import net.jini.lookup.entry.Status;
>>>>>>>>>> import net.jini.lookup.entry.UIDescriptor;
>>>>>>>>>>
>>>>>>>>>> /**
>>>>>>>>>> * A little builder utility class that creates an
array of Entry
>>>>>>>>>> classes
>>>>>>>>>> to
>>>>>>>>>> * be used as a parameter for StreamServiceRegistrar.
All the jini
>>>>>>>>>> platform
>>>>>>>>>> * Entry's are included by default.
>>>>>>>>>> *
>>>>>>>>>> * Note: This class is not threadsafe, use external
synchronization
>>>>>>>>>> if
>>>>>>>>>> required.
>>>>>>>>>> *
>>>>>>>>>> * Suggested by Dan Creswell.
>>>>>>>>>> * @author peter
>>>>>>>>>> */
>>>>>>>>>> public class DefaultEntries {
>>>>>>>>>> private final Set<Class>   entrys;
>>>>>>>>>> public DefaultEntries() {
>>>>>>>>>> entrys = new HashSet<Class>(16);
>>>>>>>>>> }
>>>>>>>>>> /**
>>>>>>>>>> * Add an Entry class.
>>>>>>>>>> * @param cl - class
>>>>>>>>>> * @return this
>>>>>>>>>> */
>>>>>>>>>> public DefaultEntries add(Class cl){
>>>>>>>>>> entrys.add(cl);
>>>>>>>>>> return this;
>>>>>>>>>> }
>>>>>>>>>> /**
>>>>>>>>>> * All all the Jini Platform Entry's
>>>>>>>>>> * @return
>>>>>>>>>> */
>>>>>>>>>> public DefaultEntries addPlatformEntries(){
>>>>>>>>>> add(Comment.class);
>>>>>>>>>> add(Location.class);
>>>>>>>>>> add(Name.class);
>>>>>>>>>> add(ServiceInfo.class);
>>>>>>>>>> add(Status.class);
>>>>>>>>>> add(UIDescriptor.class);
>>>>>>>>>> add(Address.class);
>>>>>>>>>> return this;
>>>>>>>>>> }
>>>>>>>>>> /**
>>>>>>>>>> * Remove all Entry's
>>>>>>>>>> */
>>>>>>>>>> public void reset(){
>>>>>>>>>> entrys.clear();
>>>>>>>>>> }
>>>>>>>>>> /**
>>>>>>>>>> * Generate a new array containing all Entry's added
since last
>>>>>>>>>> reset.
>>>>>>>>>> * @return
>>>>>>>>>> */
>>>>>>>>>> public Class[] getEntries(){
>>>>>>>>>> return entrys.toArray(new Class[entrys.size()]);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> @Override
>>>>>>>>>> public int hashCode() {
>>>>>>>>>> int hash = 3;
>>>>>>>>>> hash = 29 * hash + (this.entrys != null ? this.entrys.hashCode()
:
>>>>>>>>>> 0);
>>>>>>>>>> return hash;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> @Override
>>>>>>>>>> public boolean equals(Object o){
>>>>>>>>>> if (o == null) return false;
>>>>>>>>>> if (o instanceof DefaultEntries){
>>>>>>>>>> if (entrys.equals(((DefaultEntries)o).entrys)) return
true;
>>>>>>>>>> }
>>>>>>>>>> return false;
>>>>>>>>>> }
>>>>>>>>>> @Override
>>>>>>>>>> public String toString(){
>>>>>>>>>> String newline = System.getProperty("line.separator");
>>>>>>>>>> StringBuilder sb = new StringBuilder(256);
>>>>>>>>>> sb.append("DefaultEntries:");
>>>>>>>>>> sb.append(newline);
>>>>>>>>>> Iterator<Class>   it = entrys.iterator();
>>>>>>>>>> while (it.hasNext()){
>>>>>>>>>> sb.append(it.next().getName());
>>>>>>>>>> sb.append(newline);
>>>>>>>>>> }
>>>>>>>>>> return sb.toString();
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>


Mime
View raw message