CopyOnWriteArrayNavigableSet review followup

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

CopyOnWriteArrayNavigableSet review followup

Mike Duigou
Dr Heinz M. Kabutz <heinz at javaspecialists.eu> wrote:
> At the moment you can construct a CopyOnWriteArrayNavigableSet for a
> type that does not have a natural ordering and without specifying a
> comparator.  I think that makes our lives a lot harder in the
> implementation.  Better would be to force the user to pass in a correct
> Comparator for that type.  That way we know that we always have one and
> we don't need to check for null
anymore.  ... It is easy enough to pass in Comparator.naturalOrder() and
that also checks that E is in fact Comparable.

This seems like a very reasonable idea. I have always disliked the use
of null as a sentinel value.

I have also added the static create() factory methods suggested by Chris
Povirk though I did not make the constructors private as this would be
too unusual for a JDK class.

> Dr Heinz M. Kabutz <heinz at javaspecialists.eu> wrote:

> 1. In the JavaDoc comment, probably use &lt;Handler> instead of
> <Handler>

The entire block is wrapped with {@code } so this would result in &lt;
being rendered rather than <

> 2. I would probably delegate to the Integer.compare(int, int) method in
>  your Handler otherwise we might get overflows resulting in incorrect
> comparisons.

Good catch. Done.

> 3. Inside class X in the JavaDocs, use the diamond operator

Done.

> 4. Unless there is a compelling reason, I would make both comparator
> and
al fields private.

The package private access is done so that accessor methods are not
needed for BoundedNavigableSet. This is typical within the
j.u.concurrent classes.

> 5. I would make the internal constructor also private:

It is package private so that it can be called from tests.

6. Where you use the wrapped COWArrayList for locking, instead of
synchronized(super.al.lock)

You may be looking at an old implementation of COWArrayList. In the
latest JDK9 repo the type of locking has been changed to a standard Java
monitor.

> 7. I dislike turning off unchecked warnings.

Agreed. I tried your suggestion and found that warnings were still
generated for the comparator assignment so I opted to leave things as
they are/were.

> 8. Since we have type erasure in generics, it would probably be also a  
> bit more accurate to use Object[] instead of E[].  See the other
> classes in java.util.* for comparison.  I understand why you did it -
> to get Arrays.binarySort() to work.  However, it is a bit messy that
> one could construct a CopyOnWriteArrayNavigableSet with a generic type
> that is not Comparable and without a Comparator and where we would get
> an ClassCastException the moment we use it.

Agreed that the use of Object[] or E[] is inconsistent across
java.util.* Generally the policy has been to use whichever is
convenient.

> 9. The fromLoc and toLoc methods both have the same code for finding
> the
comparator

Removed by eliminating null for natural order.

> 10. I am concerned by the number of methods that are being called
> whilst holding locks.  Whilst I don't have any concrete example, I am
> concerned that this could lead to deadlocks.

We only lock on one private object from our own instance so I don't
believe there's any risk of deadlock.


Thank you for the feedback and to those who provided earlier feedback
and off-list feedback.

Once I have done some tidying and retesting I will publish an updated
version of the source.

Mike
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: CopyOnWriteArrayNavigableSet review followup

Dr Heinz M. Kabutz

> 6. Where you use the wrapped COWArrayList for locking, instead of
> synchronized(super.al.lock)
>
> You may be looking at an old implementation of COWArrayList. In the
> latest JDK9 repo the type of locking has been changed to a standard
> Java monitor.
Indeed I was - thank you for that correction.
>> 10. I am concerned by the number of methods that are being called
>> whilst holding locks.  Whilst I don't have any concrete example, I am
>> concerned that this could lead to deadlocks.
>
> We only lock on one private object from our own instance so I don't
> believe there's any risk of deadlock.
Right, in the version I was looking at you had two - the ReentrantLock
and the synchronized.  I will still need to check this more carefully,
considering how long Vector contained a deadlock even with what seemed
like a single lock :-)
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: CopyOnWriteArrayNavigableSet review followup

Peter
In reply to this post by Mike Duigou
An option might be a single threaded executor,  so all writes are uncontended and written by the same thread, then make all access volatile.


Sent from my Samsung device.
 
---- Original message ----
From: Dr Heinz M. Kabutz <[hidden email]>
Sent: 31/12/2015 09:44:54 am
To: Mike Duigou <[hidden email]>
Cc: Concurrency Interest <[hidden email]>
Subject: Re: [concurrency-interest] CopyOnWriteArrayNavigableSet review followup


> 6. Where you use the wrapped COWArrayList for locking, instead of
> synchronized(super.al.lock)
>
> You may be looking at an old implementation of COWArrayList. In the 
> latest JDK9 repo the type of locking has been changed to a standard 
> Java monitor.
Indeed I was - thank you for that correction.
>> 10. I am concerned by the number of methods that are being called 
>> whilst holding locks.  Whilst I don't have any concrete example, I am 
>> concerned that this could lead to deadlocks.
>
> We only lock on one private object from our own instance so I don't 
> believe there's any risk of deadlock.
Right, in the version I was looking at you had two - the ReentrantLock 
and the synchronized.  I will still need to check this more carefully, 
considering how long Vector contained a deadlock even with what seemed 
like a single lock :-)
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest