----- Original Message ----- > From: "Stuart Marks" <stuart.ma...@oracle.com> > To: "-" <liangchenb...@gmail.com>, "core-libs-dev" > <core-libs-dev@openjdk.java.net> > Sent: Saturday, March 5, 2022 1:58:30 AM > Subject: Re: Replace simple iterations of Map.entrySet with Map.forEach calls
> 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. Hi DrDeprecator, i wonder if we should not deprecate the method synchronized*, usually they cause more harm than good for the reason you explain. > > s'marks Rémi > > > > 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
