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

[Mike Duigou]

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.