Hi, I have to say I'm pretty skeptical of this change overall.
It sounds like the main issue is iterating over the entrySet of a map that might be
modified concurrently. The bug report says
> for concurrent maps, regular iteration of entrySet may fail spontaneously
This isn't true for ConcurrentHashMap or ConcurrentSkipListMap, which have weakly
consistent iterators that won't error out. It *is* true for synchronizedMap, which
can throw ConcurrentModificationException if the map is modified during iteration. I
guess that's mostly what you're referring to.
The problem with synchronizedMap (and the other synchronized wrappers) is that they
require external synchronization around iteration, and it's pretty much impossible
to cover all the cases where iteration occurs. It's used everywhere outside the JDK.
Even within the JDK, it appears quite frequently and is hard to avoid. For example,
consider
collection.addAll(synchronizedMap.keySet())
AbstractCollection::addAll iterates its argument, and this is inherited in a bunch
of places.
The root of the problem is that the synchronized wrappers support concurrent
updating only in a very narrow range of use cases. If people have trouble with
concurrent map updating, they need to use a real concurrent data structure, or they
need to put everything inside their own class and have it do locking around
higher-level operations. Replacing a few entrySet iteration cases with Map::forEach
doesn't help anything.
s'marks
On 2/22/22 3:09 PM, - wrote:
Hello,
In the patch for 8281631 <https://bugs.openjdk.java.net/browse/JDK-8281631>,
xenoamess intentionally avoided
<https://github.com/openjdk/jdk/pull/7431#discussion_r810666916> repeatedly
calling Map::size, for it may not be constant-timed due to being
concurrent. This alerted me that the for loops on Map::entrySet may be
similarly not thread-safe.
I believe that we should migrate for loop on Map::entrySet to Map::forEach
calls; concurrent map callees usually have dedicated logic
<https://github.com/openjdk/jdk/blob/2557ef8a02fe19784bd5e605b11d6bd574cde2c2/src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java#L119>
to mitigate such thread-safety issues. Synchronized map
<https://github.com/openjdk/jdk/blob/2557ef8a02fe19784bd5e605b11d6bd574cde2c2/src/java.base/share/classes/java/util/Collections.java#L2735>
callees are benefitted too.
Another lesser benefit is reduced object allocation for iteration. While
the most popular implementations (HashMap, LinkedHashMap, WeakHashMap,
TreeMap) don't need to allocate entries for iteration, many other (EnumMap,
IdentityHashMap, concurrent maps) do, and iterating those maps with forEach
is less costly. (Note that 8170826
<https://bugs.openjdk.java.net/browse/JDK-8170826> takes care of
implementing proper forEach in EnumMap)
A JBS issue has already been submitted at
https://bugs.openjdk.java.net/browse/JDK-8282178, and I have prepared a
patch. This patch modifies the putAll implementations of a few JDK maps to
let the callee feed entries through Map::forEach to be put into the maps.
Two places of Map::entrySet calls in Collectors has also been updated.
For most other existing usages in the JDK, I don't think they will benefit
from such a migration: They mostly iterate on the entryset of the popular
implementations that already host entries and are single-threaded, and I
don't think it's in our best interest to touch those use cases.
So, here are my questions:
1. Is such a concept of replacing Map::entrySet calls with Map::forEach
calls reasonable, or is there any fundamental flaw?
If the concept sounds good, I will submit a patch, so we can answer
2. Is the changes implemented correct for this concept, i.e. targeting code
where users supply callee maps?
Best regards
