8020860: cluster Hashtable/Vector field updates for better transactional memory behaviour

[Martin Buchholz]

Did you mean s/code/link/ ?

+ * The Enumerations returned by Hashtable's {@code #keys keys} and
+ * {@code #elements elements} methods are <em>not</em> fail-fast.




On Tue, Apr 15, 2014 at 3:14 PM, Mike Duigou <mike.duigou at oracle.com> wrote:

>
> On Apr 14 2014, at 18:25 , Martin Buchholz <martinrb at google.com> wrote:
>
> > I'll retreat to being neutral on the overall idea.
> >
> > In general, it *is* a best software engineering practice to do all the
> reading and computing before doing all the writing at the end.
> >
> > You'll break anyone who does the crazy thing of intentionally calling
> add(null, Integer.MAX_VALUE) just for the side effect of incrementing
> modCount.  How would _you_ increment modCount without doing any modding?!
>
> Assuming you meant Vector::put(Integer.MAX_VALUE, null). You could use
> synchronized(vector) { vector.removeRange(vector.size(), vector.size()) }
> with slight higher cost.
>
> > You could make a real improvement (more concurrency) for addAll, by
> moving the call to toArray out of the synchronized block.
> >      public synchronized boolean addAll(Collection<? extends E> c) {
> > -        modCount++;
> >          Object[] a = c.toArray();
>
> Done!
>
> >
> > It's hardly worth it for e.g. clear, where you are doing nothing but
> writes in the loop as well.
> >      public synchronized void clear() {
> >          Entry<?,?> tab[] = table;
> > -        modCount++;
> >          for (int index = tab.length; --index >= 0; )
> >              tab[index] = null;
> > +        modCount++;
> >          count = 0;
> >      }
>
> For consistency with other methods I retained the movement of modCount.
>
> I have updated the webrev with what I hope is the final form:
>
> http://cr.openjdk.java.net/~mduigou/JDK-8020860/1/webrev/
>
> Good to go?
>
> Mike
>
> PS:- I would have liked to rewritten Hashtable::putAll(Map<> t)
>
> as :
>
> {
>    t.forEach(this::put)
> }
>
> but decided against this since it may change the synchronization on t
> which seems risky. ie. some Thread could hold a lock on t which would now
> deadlock where the current implementation does not. The forEach()
> implementation would have advantages as for some cases it would avoid the
> need to create Map.Entry objects in the iterator.
>
>