Is this synchronization on ConcurrentHashMap correct?

Question

I have a key-value map accessed by multiple threads:

private final ConcurrentMap<Key, VersionValue> key_vval_map = new ConcurrentHashMap<Key, VersionValue>();

My custom get() and put() methods follow the typical check-then-act pattern. Therefore, synchronization is necessary to ensure atomicity. To avoid locking the whole ConcurrentHashMap, I define:

private final Object[] locks = new Object[10];
{
    for(int i = 0; i < locks.length; i++) 
        locks[i] = new Object();
}

And the get() method goes (it calls the get() method of ConcurrentHashMap):

public VersionValue get(Key key)
{
    final int hash = key.hashCode() & 0x7FFFFFFF;

    synchronized (locks[hash % locks.length])   // I am not sure whether this synchronization is necessary.
    {
        VersionValue vval = this.key_vval_map.get(key);
        if (vval == null)   
            return VersionValue.RESERVED_VERSIONVALUE;  // RESERVED_VERSIONVALUE is defined elsewhere
        return vval;
    }
}

The put() method goes (it calls the get() method above):

public void put(Key key, VersionValue vval)
{
    final int hash = key.hashCode() & 0x7FFFFFFF;

    synchronized (locks[hash % locks.length])   // allowing concurrent writers
    {
        VersionValue current_vval = this.get(key);  // call the get() method above

        if (current_vval.compareTo(vval) < 0)   //  it is an newer VersionValue
            this.key_vval_map.put(key, vval);
    }
}

The above code works. But, as you know, working is far from being correct in multi-threaded programming.

My questions are :

1. Is this synchronization mechanism (especially synchronized (locks[hash % locks.length])) necessary and correct in my code?

2. In Javadoc on Interface Lock, it says

Lock implementations provide more extensive locking operations than can be obtained using synchronized methods and statements.

Then is it feasible to replace synchronization by Lock in my code?

Edit: If you are using Java-8, don't hesitate to refer to the answer by @nosid.

Answer 1

ConcurrentMap allows you to use optimistic locking instead of explicit synchronization:

VersionValue current_vval = null;
VersionValue new_vval = null;

do {
    current_vval = key_vval_map.get(key);

    VersionValue effectiveVval = current_vval == null ? VersionValue.RESERVED_VERSIONVALUE : current_vval;

    if (effectiveVval.compareTo(vval) < 0) {
        new_vval = vval;
    } else {
        break;
    }
} while (!replace(key, current_vval, new_vval));

...

private boolean replace(Key key, VersionValue current, VersionValue newValue) {
    if (current == null) {
        return key_vval_map.putIfAbsent(key, newValue) == null;
    } else {
        return key_vval_map.replace(key, current, newValue);
    }
}

It will probably have better performance under low contention.

Regarding your questions:

1. If you use Guava, take a look at Striped
2. No, you don't need additional functionality of Lock here