river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gregg Wonderly <gr...@wonderly.org>
Subject Re: MarshalledServiceItem
Date Tue, 08 Feb 2011 17:08:58 GMT
On 2/8/2011 10:51 AM, Patricia Shanahan wrote:
> 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.

Yes, this is true.  If we do need some compound operations, then it would be 
necessary to add a lock to synchronize on, and then everything else referencing 
the structure would need to be synchronized since CHM uses internal locking 
which does not involve synchronized(this).

Gregg Wonderly

>
> 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