0

Java's SynchronizedMap, which is created by calling Collections.synchronizedMap() seems to be using a single mutex for all of the map's operations. For instance:

...

public V get(Object key) {
    synchronized (mutex) {return m.get(key);}
}

public V put(K key, V value) {
    synchronized (mutex) {return m.put(key, value);}
}

...

Why did the authors choose to use a single mutex for both read and write operations? Wouldn't a ReadWriteLock be more suitable here?

Malt
  • 28,965
  • 9
  • 65
  • 105
  • That class has been obsolete since java 5 (not sure if formally or just practically). ConcurrentMap should be vastly more efficient than either of the two approaches so there's just no reason to invest time in improving the code. But yes you're right that it'd be much better. – Voo Apr 09 '17 at 19:34
  • Generally, I agree. But there's no direct concurrent alternative for the LinkedHashMap within the JDK as far as I know. It's a quick and easy way of creating an LRU cache without external libraries. The only way to make it thread safe is with locks. – Malt Apr 09 '17 at 19:51

1 Answers1

4

I think this is done for historical reasons more than anything: if you look carefully, ReadWriteLock was added in JDK version 1.5, while SynchronizedMap was part of JDK since 1.2.

If we think in terms behavioral compatibility, it's invalid to suddenly change how synchronized map works internally (i.e. by ordering both reads and writes), because there would be a lot of programs written between 1.2 and 1.5 that would've relied on exactly this behavior. Another point to this is - the Map implementation is called synchronized, which actually does mean that it orders every access, just as synchronized keyword does.

There's even more to this: read-write locks come in two flavors of sync: fair and non-fair. If we will replace simple mutex with read-write lock, which flavor we go for? What about different lock implementations? Etc.

It honestly makes more sense to provide a general-use concurrent map as they did with ConcurrentHashMap, and everything else will be implemented by library writers if developers need it.

M. Prokhorov
  • 3,894
  • 25
  • 39
  • Can you show one example where using the rw lock would lead to behaviour that would not been possible beforehand? Because I don't see how that'd be possible. It's more likely that it's just not worth changing the behaviour considering there's no good reason to use the class for a long time. – Voo Apr 09 '17 at 18:29
  • (It's actually quite simple to show that the behaviour is non distinguishable I think: Assume that you have n threads reading. With the new behaviour this can happen concurrently. With the old behaviour you could interrupt every thread right after it unlocks the mutex but before it returns from the method. After all threads have read, let them continue concurrently. The externally visible behaviour is identical to the new one.) – Voo Apr 09 '17 at 19:31
  • 1
    @Voo, no, a read-write lock is designed specifically to allow multiple concurrent read and single blocking write. Synchronized map, due to its design, only allows single read _or_ write. – M. Prokhorov Apr 09 '17 at 19:35
  • That's missing the point. You have to show some *externally observable* behavior that can occur when using RW locks that cannot happen under any circumstance when using synchronized blocks. And since the above scenario I outline shows that you cannot distinguish multiple concurrent reads from a scenario with only a single reader under a very simple scheduling I cannot see how this would be possible. – Voo Apr 09 '17 at 20:10
  • This is one example of why it's so important to clearly distinguish between what is part of the API contract and what is an implementation detail. The contract only specifies the externally visible behavior, it doesn't care how you achieve this. And performance is generally not part of the API contract (apart from specifying some asymptotic complexities for operations on data structures). – Voo Apr 09 '17 at 20:16
  • @Voo documentation describes possibility of writes being starved when guarded structure is being hammered by large number of readers, because writes are required exclusive access, and reads are being constantly reacquired. Synchronized block will not be a subject to this, since all waiters are in the same queue. – M. Prokhorov Apr 09 '17 at 20:17
  • `synchronized` is not a fair lock, so the same thing can happen there. Thinking of it as a "queue" is a bad idea (fair locks are a horrible idea in general and should in almost all situations be avoided). But yes you'd definitely want a bit more finesse for a practical implementation with a RW lock - but there are many solutions for the starvation problem. – Voo Apr 09 '17 at 20:18
  • But mutex can starve any access, while rwl starves only one or the other. – M. Prokhorov Apr 09 '17 at 20:20
  • I'm not sure I understand where you're going with this, because it's quite trivial to come up with one scheduling that demonstrates equal behavior. If you want to show that two implementations implement different API contracts, you have to show one possible execution path that shows externally visible behavior that is possible with one implementation but not the other. – Voo Apr 09 '17 at 20:23
  • Yeah, but can you _prove that all possible uses show **same** behavior_? No, because various access starvations are exactly what sets them apart. Switching to use new APIs is an opt-in by application developer. Changing how syncMap works internally affects backwards compatibility for apps that have been reasoned about using old way of working. – M. Prokhorov Apr 09 '17 at 20:27
  • Well no you can't prove that. But that's not how you define API compatibility anyhow, because that wouldn't make any sense. After all, running your application twice without changing the code can already lead to two completely different execution behaviors - so your application is already broken, independent which implementation you use. It is perfectly acceptable and has been done in the past to switch two implementations if their contractual behavior is identical, if one implementation is superior to the other. – Voo Apr 09 '17 at 20:29
  • You have to understand that "old way of working" and "new way of working" have the exact identical externally visible behavior. They are isomorph. If your reasoning guaranteed that your app worked correctly with the old implementation, it is guaranteed to work with the new one. If your application is broken with the new implementation, it was just as broken with the old one. – Voo Apr 09 '17 at 20:34
  • No, running application with threading will not change its behavior, because it's behavior explicitly defined that specific ordering of certain operations if undefined. Judging only by API compatibility - there's only ever a single behavior that one can expect of the Map - and that is defined by `java.util.Map` interface, which syncMap follows, by the way. – M. Prokhorov Apr 09 '17 at 20:35
  • It's very, very simple: Show a single scenario that you think can happen when using a RW lock that cannot happen when using a synchronized block. If you cannot do that, the two implementations are identical. For example contrary to your claim there's a *BIG* difference between the API of `java.util.Map` and `SynchronizedMap`: It is perfectly legal for two threads to call get() on a synchronized map without interthread synchronization, while this is undefined behavior for Map. So here we have a simple scenario that shows visible differing behavior, hence they're not isomorph. – Voo Apr 09 '17 at 20:38
  • @Voo, sorry, but unfortunately, the "let it burn" approach was never the Java way. People still have to work with code written 15-some years ago, and there's not enough justification in the world for Oracle to turn around and say "sorry, guys, we've changed our implementations now, and your previously working application may start to produce unexpected results starting tomorrow. better get on it and fix it okbyexXx". That stuff loses you customers. – M. Prokhorov Apr 09 '17 at 20:39
  • You're missing the point: If your application was working perfectly beforehand it would work just as well with the changed implementation. If it was already broken, it would be just as broken beforehand. If it would produce unexpected results after the change, it was producing unexpected results beforehand. And goodness, Oracle and Sun before have changed implementation details in HotSpot so many times that's really not true that they'd never update anything (go ahead and look up what the current default GC is and what it used to be with HotSpot 1.0). – Voo Apr 09 '17 at 20:43
  • Ok, so hear me out: we have an access starvation problem in the Map under heavy load, where statistically there are 40 reads to one write. Previously under that load there were roughly 1/40 change that a write gets through. We change implementation, and now it so happens that read lock almost never gets released, so we get more like 1/400 change to let a write through, possibly even less. Was it broken before? Sure, it was. What is your justification of breaking it even more though? – M. Prokhorov Apr 09 '17 at 20:50
  • Now you're asking a different question: How much leeway do you want to give broken applications that rely on implementation details? This is now an engineering trade-off in which many variables flow into, with no simple answer. That question has been asked in many contexts over the years and the answer has varied. Although in this case there are simple solutions that work quite well (look up `PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP` in posix for the most important part of that puzzle). – Voo Apr 09 '17 at 20:55