On Oct 10 2013, at 06:37 , Paul Sandoz wrote: > Hi Mike, > > This looks like a good direction. Need to look more closely. > > I wonder if we even require such detailed support to throw CME?
Some additional CME will be thrown indirectly by iteration. This applies to forEach/replaceAll. Virtually all the cases where CME is thrown in the "new" Map defaults are the points where previously the implementation would have looped/retried. > For the cases where no function values are passed it has very limited value, > we know it is effectively useless for non-current collections being modified > concurrently. Understood. The alternative would be to GIGO these situations, return and ignore them. > For the cases where a function value is passed and it modifies the map then > it could have some value. But i wonder, for any non-looping function value > accepting method on Map (anything other than forEach/replaceAll), whether it > is just simpler to state: "if a function value modifies the entry under > computation then this method may return incorrect results`". Modification of any other entry may have the same result. I suspect that modification by some other thread is as likely to be a problem as modification by the function. Any thoughts Doug? Is this advancing or retreating from the right direction? Mike > > Paul. > > On Oct 10, 2013, at 5:46 AM, Mike Duigou <mike.dui...@oracle.com> wrote: > >> >> On Oct 8 2013, at 01:27 , Paul Sandoz wrote: >> >>> Hi Mike, >>> >>> I am probably going over old ground here... >>> >>> Given that there is ConcurrentMap is there any point in having the defaults >>> on Map detect concurrent modification and barf, or retry, or neither of the >>> previous two e.g. putIfAbsent, computeIfPresent and replace respectively >>> i.e. there seems to be inconsistent behaviour here. >>> >>> Would it not be better to separate concerns of serial and concurrent access >>> in the defaults of Map and ConcurrentMap? >> >> I have created a prototype renovated Map and ConcurrentMap to provide >> (mostly) separate implementations. >> >> http://cr.openjdk.java.net/~mduigou/JDK-8024688.2/0/webrev/ >> >> This passes all of the existing regression tests and I believe is >> performance neutral. The implementations are now more relatively correct for >> both flavours. ie. Map defaults don't retry and throw CME when they detect >> that something changed unexpectedly. The ConcurrentMap defaults were mostly >> just moving over the prior Map defaults but I also removed a few cases of >> null value handling that were no longer needed. >> >> I've wanted to explore this for a while (since June) but haven't previously >> had a chance. I'm somewhat convinced that this is the right direction to be >> going but I am sure further refinement is possible (particularly in regards >> to the spec). >> >>> The defaults on Map could state they are not suitable for maps that can be >>> concurrently modified, if that is required extend from ConcurrentMap. If >>> function values passed to methods modify the Map then undefined behaviour >>> will result. (I am also wondering if there are currently edge cases e.g. if >>> a function value modifies the source then the re-try loop will never >>> terminate.) >>> >>> I realize that is separate from the null behaviour, but perhaps this >>> separate will help clarify null behaviour? >> >> I realized that some of the statements about "implementations must document >> how they handle nulls with this method" were no longer relevant. The null >> handling behaviour was entirely covered by the class documentation and the >> method specification. This is mostly a consequence of having added "or is >> mapped to null value" in couple of cases later on in the process. >> >> Thanks for pushing on this point (and David Holmes for earlier suggestion >> that this might be important) >> >> Mike >> >>> Paul. >>> >>> On Oct 4, 2013, at 5:35 AM, Mike Duigou <mike.dui...@oracle.com> wrote: >>> >>>> Hello all; >>>> >>>> This is a changeset which improves the consistency of several Map.merge >>>> implementations for handling of null values. >>>> >>>> The existing unit tests hadn't considered several cases where the result >>>> of the remapper was not the same as the value. I've restructured the merge >>>> tests to be more thorough and systematic this revealed a couple of >>>> problems. >>>> >>>> http://cr.openjdk.java.net/~mduigou/JDK-8024688/0/webrev/ >>>> >>>> Like several of the previous patches, this one introduces an alternative >>>> default for ConcurrentMap to work around issues involving null values >>>> where the handling in the general Map default would be incorrect. >>>> >>>> Mike >>> >> >
