views:

274

answers:

2

I'd like to be able to conditionally replace a value in a ConcurrentHashMap. That is, given:

public class PriceTick {
   final String instrumentId;
   ...
   final long timestamp;
   ...

And a class (let's call it TickHolder) which owns a ConcurrentHashMap (let's just call it map).

I wish to be able to implement a conditional put method, so that if there's no entry for the key, the new one is inserted, but if there is an existing entry, the new one is inserted only if the timestamp value in the new PriceTick is greater than the existing one.

For an old-school HashMap solution, TickHolder would have a put method:

public void add(PriceTick tick) {
   synchronized(map) {
      if ((map.get(tick.instrumentId) == null)
         || (tick.getTimestamp() > map.get(tick.instrumentId).getTimestamp()) )
         map.put(tick.instrumentId, tick);
      }
}

With a ConcurrentHashMap, one would want to drop the synchronization and use some atomic method like replace, but that's unconditional. So clearly the "conditional replace" method must be written.

However, since the test-and-replace operation is non-atomic, in order to be thread safe, it would have to be synchronized - but my initial reading of the ConcurrentHashMap source leads me to think that external synchronization and their internal locks will not work very well, so at a very minimum, every Map method which performs structural changes and the containing class performs would have to be synchronized by the containing class... and even then, I'm going to be fairly uneasy.

I thought about subclassing ConcurrentHashMap, but that seems to be impossible. It makes use of an inner final class HashEntry with default access, so although ConcurrentHashMap is not final, it's not extensible.

Which seems to mean that I have to fall back to implementing TickHolder as containing an old-school HashMap in order to write my conditional replace method.

So, the questions: am I right about the above? Have I (hopefully) missed something, whether obvious or subtle, which would lead to a different conclusion? I'd really like to be able to make use of that lovely striped locking mechanism here.

+5  A: 

The non-deterministic solution is to loop replace():

do {
  PriceTick oldTick = map.get(newTick.getInstrumentId());
} while ((oldTick == null || oldTick.before(newTick)) && !map.replace(newTick.getInstrumentId(), oldTick, newTick);

Odd though it may seem, that is a commonly suggested pattern for this kind of thing.

cletus
Interesting. When you say "non-deterministic", are you saying that the outcome is non-deterministic? Or that the timing is? Also, am I right that your "before" method is meant to stand for my timestamp comparison? Or am I missing something?
CPerkins
By non-deterministic I mean that it may take several iterations to do the replacement because the get and replace obviously isn't an atomic operation but it is safe.
cletus
Got it. Thanks. A deterministic outcome from a non-deterministic method (with obviously non-deterministic timing). Reminds me of TCP/IP. :) Thanks, great idea.
CPerkins
+1  A: 

As an alternative, could you create a TickHolder class, and use that as the value in your map? It makes the map slightly more cumbersome to use (getting a value is now map.getValue(key).getTick()), but it lets you keep the ConcurrentHashMap's behavior.

public class TickHolder {
  public PriceTick getTick() { /* returns current value */
  public synchronized PriceTick replaceIfNewer (PriceTick pCandidate) { /* does your check */ }
}

And your put method becomes something like:

public void updateTick (PriceTick pTick) {
  TickHolder value = map.getValue(pTick.getInstrumentId());
  if (value != null) {
    TickHolder newTick = new TickHolder(pTick);
    value = map.putIfAbsent(pTick.getInstrumentId(), newTick);
    if (value == null) {
      value = newTick;
    }
  }
  value.replaceIfNewer(pTick);
}
Sbodd
Interesting. Did you mean to synchronize PriceTick? How does that synchronization play with the locking mechanism inside ConcurentHashMap?
CPerkins
TickHolder.replaceIfNewer would need to be synchronized - that's basically where your atomic test-and-conditionally-update operation goes. There's no direct interaction between the synchronization on TickHolder.replaceIfNewer and the map's synchronization. The map (with the little bit of logic in updateTick) makes sure there's never more than one TickHolder for a given key; the replaceIfNewer synchronization makes sure that updates to a given TickHolder don't have threading issues.Neither updateTick nor PriceTick itself (assuming PriceTick is immutable) would need any other synchronization
Sbodd
I'm a bit confused as to why you think there's no need for other synchronization. The synchronization of replaceIfNewer only limits calls to that method. So other threads can make structural changes to the map using other methods, no? Still, it's an interesting approach. I will think on it later.
CPerkins
I guess I was making an implicit assumption that nothing ever gets removed from your map - all writes go through updateTick. In which case, once you've gotten the TickHolder for a given InstrumentId, you no longer care about other structural changes to the map; the TickHolder you have is the value in the Map for your InstrumentId, and you just need your operation to be mutually exclusive with other operations on that InstrumentId.
Sbodd
Ticks do have to be removed, though I didn't say that. Note, though, that it's not just removals - adds also can cause structural changes (e.g., rebucketing).
CPerkins