views:

161

answers:

3

I would like to collect some metrics from various places in a web app. To keep it simple, all these will be counters and therefore the only modifier operation is to increment them by 1.

The increments will be concurrent and often. The reads (dumping the stats) is a rare operation.

I was thinking to use a ConcurrentHashMap. The issue is how to increment the counters correctly. Since the map doesn't have an "increment" operation, I need to read the current value first, increment it than put the new value in the map. Without more code, this is not an atomic operation.

Is it possible to achieve this without synchronization (which would defeat the purpose of the ConcurrentHashMap)? Do I need to look at Google Collections ?

Thanks for any pointers.

PS There is a related question on SO (http://stackoverflow.com/questions/81346/most-efficient-way-to-increment-a-map-value-in-java) but focused on performance and not multi-threading

EDIT: For those arriving here through searches on the same topic: besides the answers below, there's a useful presentation which incidentally covers the same topic. See slides 24-33.

+2  A: 

Please take a look at this thread http://stackoverflow.com/questions/2539654/java-concurrency-many-writers-one-reader/2539761#2539761

The answer I gave would probably be the most efficient way to increment a value pulled from a ConcurrentHashMap

John V.
@John That's indeed pretty much what I needed. Steve described the same idea, but so far your answer is more direct.
wishihadabettername
+7  A: 

You're pretty close. Why don't you try something like a ConcurrentHashMap<Key, AtomicLong>? If your Keys (metrics) are unchanging, you could even just use a standard HashMap (they are threadsafe if readonly, but you'd be well advised to make this explicit with an ImmutableMap from Google Collections or Collections.unmodifiableMap, etc.).

This way, you can use map.get(myKey).incrementAndGet() to bump statistics.

Steven Schlansker
Just don't forget to store the `HashMap` in a `final` member. And better wrap the map in a `unmodifiable` wrapper. Better yet, you can use `ImmutableMap` from Guava (superset of google collection) and it should be really really fast.
Enno Shioji
@Zwei: good point, edited the answer to include that advice :)
Steven Schlansker
@Steven. The list of metrics is built as they supply data (i.e. the map's keys will be added as the system runs and various collection points are hit; building the list a priori would be error-prone). I forgot about AtomicLong's incrementAndGet(), it's just what I need. If it didn't exist, I was thinking that another approach would have been for the metrics collectors not to increase the counters, but simply to add the request to do so in a queue maintained by the singleton. Thus, the callers only add() to a list, which periodically is read and processed. Not as simple, though.
wishihadabettername
+2  A: 

Other than going with AtomicLong, you can do the usual cas-loop thing:

private final ConcurrentMap<Key,Long> counts =
    new ConcurrentHashMap<Key,Long>();

public void increment(Key key) {
    if (counts.putIfAbsent(key, 1)) == null) {
        return;
    }

    Long old;
    do {
       old = counts.get(key);
    } while (!counts.replace(key, old, old+1)); // Assumes no removal.
}

(I've not written a do-while loop for ages.)

For small values the Long will probably be "cached". For longer values, it may require allocation. But the allocations are actually extremely fast (and you can cache further) - depends upon what you expect, in the worst case.

Tom Hawtin - tackline
I think that's right. Apologies to anyone looking at the previous broken code. (You could rearrange it so that it starts off with a `get`, a compare to null and only then try `putIfAbsent`, continuing with a normal `while` loop.)
Tom Hawtin - tackline