2

I have this class with 3 read methods and 1 write method:

class ResourceClass {

  private static Map resourceMap = new HashMap();

  // New method to update resource
  public void write(String key, Object resource) {
    resourceMap.put(key, resource);
  }

  public Object read(String var1) {
    return resourceMap.get(var1);
  }

  public Object read(String var1, String var2) {
    // .. Do task with var1 and var2
    return resourceMap.get(var1);
  }

  public Object read(String var1, String var2, String var3) {
    // .. Do task with var1, var2 and var3
    return resourceMap.get(var1);
  }  
}

Currently, this class only contains a write and 3 read methods to consume the static resource. The problem with this configuration is that the only way to update the resourceMap is to restart the application so the ResourceClass is created again the resourceMap is added to the class for its consumption.

What I want is to add some dynamic way to update resourceMap without restarting the service, but for that I have to make this class Thread-Safe, in order to handle a write method to update the resourceMap safely. For this I have the option to use synchronized keyword in read and write methods so only one thread has access to resourceMap. This approach solves the problem, but includes others as well. These read methods are high-concurrent methods so adding a synchronized keyword will impact the service performance dramatically and surely we don't want that.

Does any body knows a way to keep the threads reading (not blocking each other) but when there comes one thread to write all read methods wait for the write to finish and resume when the write finishes?

Joe Almore
  • 4,036
  • 9
  • 52
  • 77
  • 2
    Have you looked into [ReentrantReadWriteLock](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html)? – sstan Dec 26 '15 at 20:55
  • 2
    BTW, the fact that `resourceMap` is *static*, but it gets assigned/read/modified by *instance* methods, that seems wrong. I have to imagine that you are using this class as a sort of singleton, but the design doesn't enforce that, so it looks broken to me. What happens to `resourceMap` when you instantiate `ResourceClass` a 2nd time? – sstan Dec 26 '15 at 21:00
  • Well, it is not actually assigned in the constructor, it only has an `addResource()` method which is only called during the service initialization. `ResourceClass` is never instantiated. BTW, just read about `ReadWriteLock` and seems exactly what I am looking for, I'll give it a try and see how it performs. – Joe Almore Dec 26 '15 at 21:04
  • 2
    You could use a [`ConcurrentHashMap`](https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html) as the backing map. – Mick Mnemonic Dec 26 '15 at 21:11
  • 1
    What do you mean, "`ResourceClass` is never instantiated"? The methods you've shown are all instance methods -- they can't be called without instantiating it! – ruakh Dec 26 '15 at 22:17

2 Answers2

3

As @Mike Mnomonic said in the comments, ConcurrentHashMap is a thread-safe map with a tunable concurrency level. As with other hash maps, ConcurrentHashMap has a backing array; this backing array is split into several sub-arrays depending on your specified concurrency level (default 16) with one lock per sub-array, for example if you have a capacity of 128 and are using the default concurrency level of 16 then sub-array [0,8) has its own lock, [8, 16) has its own lock, [16, 24) has its own lock, and so forth, so two threads can write to two different sub-arrays without blocking each other.

If writes are very infrequent then you may get better performance with an ImmutableMap wrapped in an AtomicReference.

private final AtomicReference<ImmutableMap<String, Object>> resourceMap;

public void write(String key, Object value) {
    boolean success = false;
    while(!success) {
        ImmutableMap oldMap = resourceMap;
        ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
        builder.putAll(resourceMap.entrySet());
        builder.put(key, value);
        success = resourceMap.compareAndSet(oldMap, builder.build());
    }
}

public Object read(String var1) {
    ...
    return resourceMap.get().get(var1); // get map, then get value
}
public Object read(String var1, String var2);
public Object read(String var1, String var2, String var3);

On a write you're copying the old map, adding the new value, and swapping the new map for the old map; if two updates try to stomp on each other then only one will succeed, meanwhile the other will keep retrying until its update sticks. The readers don't need to care about any of this - they just get the current map.

Zim-Zam O'Pootertoot
  • 17,888
  • 4
  • 41
  • 69
  • Thanks for your answer. The theory for `ConcurrentHashMap` is interesting. Nevertheless, I have to add that actually the writes are very infrequent since it only happens when a new module is added to the service and not during the operation of the service. – Joe Almore Dec 26 '15 at 21:42
  • @JoeAlmore See my updated answer about using a `volatile ImmutableMap`, which should give good read performance at the cost of worse write performance. I agree that a `ConcurrentHashMap` is not appropriate if updates are very infrequent. – Zim-Zam O'Pootertoot Dec 26 '15 at 22:07
  • 3
    Re: "`volatile` carries less of a performance hit on reads [than `AtomicReference`]": There are probably some architectures where this is the case, but I think on *typical* architectures there's actually no difference. (+1 for the immutable+volatile approach, by the way. I've used that approach a few times, with good results.) – ruakh Dec 26 '15 at 22:23
  • @ruakh I've replaced `synchronized` with a `compareAndSet` in my answer. I prefer atomic references over volatile references for clarity/consistency, but was under the impression that the JIT wasn't able to optimize atomic references as well as it could optimize volatiles for whatever reason – Zim-Zam O'Pootertoot Dec 26 '15 at 23:30
  • As far as I know a reference assignment (i.e., `=`) is atomic as well. So, why not better use a reference assignment instead of an `Atomic`? And also mark it `volatile`. – Joe Almore Dec 26 '15 at 23:46
  • 1
    @JoeAlmore `volatile` tells the compiler/virtual machine that all threads should see the updated value of the object's field; otherwise e.g. a thread might cache the field's value or the VM might reorder a load/store or whatever and things could break. The `compareAndSet` ensures that if two writers try to update the map at the same time then one will succeed while the other will fail and then retry, whereas using a standard assignment could result in one of the updates being lost. – Zim-Zam O'Pootertoot Dec 27 '15 at 00:13
1

For highly read heavy loads, consider cloning the collection and adding to the clone and then assigning the result to the field.

If the write methods may be called concurrently, work may have to be done to ensure that access to this method is 'thread safe'.

Rich O'Kelly
  • 41,274
  • 9
  • 83
  • 114
  • 2
    I think you're implying that this strategy would avoid needing to synchronize readers. But to guarantee that readers see the new cloned collection after reassignment, some form of memory synchronization is still needed. – sstan Dec 26 '15 at 21:16
  • @sstan yep - that's the rationale behind the approach. No synchronisation is necessary - the spec guarantees it (outside of the CPU reordering reads and writes - but this wouldn't have a necessarily adverse effect in this example). – Rich O'Kelly Dec 26 '15 at 21:38
  • 1
    Maybe you can share the part of the spec you are referring to. My understanding is that each thread has its own copy of memory, and for a reader thread to be able to see the changes made by a writer thread, some synchronization or memory barrier is necessary. Otherwise, a reader thread *could* keep reading from a stale copy of memory indefinitely. – sstan Dec 26 '15 at 21:53
  • [Can a C# thread really cache a value and ignore changes to that value on other threads?](http://stackoverflow.com/questions/458173/can-a-c-sharp-thread-really-cache-a-value-and-ignore-changes-to-that-value-on-ot) – sstan Dec 26 '15 at 22:10
  • @sstan Yes, this approach may indeed need some sort of synchronization, so, correct me if I'm wrong, but in that case you could mark it as `volatile` so other threads have access to the new reference assignation. – Joe Almore Dec 26 '15 at 23:55
  • 1
    @sstan Apologies - you are correct, there *could* be a case whereby the values are never synchronised across threads (assumes zero synchronised blocks being hit on a thread). As JoeAlmore points out, marking the field as volatile would suffice, alternatively, using a pre-baked implementation of a copy on write collection may be a sound way to go. – Rich O'Kelly Dec 28 '15 at 22:56