On Apr 19, 2013, at 2:42 PM, David Holmes <[email protected]> wrote:
> On 19/04/2013 10:14 PM, Paul Sandoz wrote: >> >> On Apr 19, 2013, at 1:15 PM, Alan Bateman <[email protected]> wrote: >> >>> On 18/04/2013 19:49, Akhil Arora wrote: >>>> Looks like the stars are aligning on getting on this into TL... the >>>> refreshed webrev is - >>>> >>>> http://cr.openjdk.java.net/~akhil/8001647.8/webrev/ >>>> >>> A minor comment on Collection.removeIf is "that satisifies the given >>> predicate" might be better than "which matches the provided predicate". >>> Also for completeness, you could say "RuntimeExceptions and Errors thrown >>> by the predicate are propagated ...". >>> >>> In List.replaceAll then @throws NullPointerException is listed twice, which >>> is okay, but might be better to combine them. A typo in the second NPE >>> description "if the an element". >>> >>> In the implementation then the only thing that puzzled me is checking the >>> modification count in legacy Vector, that seems unnecessary. >>> >> >> The function value could structurally modify the Vector instance. > > So this came through while I was writing my similar comments ... > > My reaction to this is simply "well don't do that". FWIW in the past this functionality has helped me detect bugs, so yes i agree "don't do it" but sometimes it unintentionally happens and it can be harder to track down the source of the problem without a CME. > If the function/predicate/comparator is mutating the Vector then the user > gets what they deserve in my opinion. Trying to account for that is somewhat > futile. As per my other email the loop check for modCount==expectedModCount > will get hoisted from the loop. Further in removeIf you need to be a lot more > defensive during the first iteration as you haven't kept a reference to the > original size and array. That aside the second part of removeIf (the actual > removal) doesn't invoke any external code so no concurrent modification is > possible then. > > This seems like overkill to me. > I think the mod count checking should be explicitly hoisted outside the loops and performed at the end of the loop/method (see the spliterator.forEachReamining implementations) since a CME should only be used to detect bugs. Doug's comments on ArrayList's spliterator [*] are relevant. Probably the most important methods for such checking are forEach, to be consistent with the default method implementation, Iterator/Spliterator.forEachRemaining, and Stream.forEach. If we can do such checking easily and efficiently for the other methods, which seems possible, then i think it worth doing and is consistent with the default methods, since they use iterator (List.sort defers to Collections.sort uses the iterator to update the list). Paul. [*] /* * If ArrayLists were immutable, or structurally immutable (no * adds, removes, etc), we could implement their spliterators * with Arrays.spliterator. Instead we detect as much * interference during traversal as practical without * sacrificing much performance. We rely primarily on * modCounts. These are not guaranteed to detect concurrency * violations, and are sometimes overly conservative about * within-thread interference, but detect enough problems to * be worthwhile in practice. To carry this out, we (1) lazily * initialize fence and expectedModCount until the latest * point that we need to commit to the state we are checking * against; thus improving precision. (This doesn't apply to * SubLists, that create spliterators with current non-lazy * values). (2) We perform only a single * ConcurrentModificationException check at the end of forEach * (the most performance-sensitive method). When using forEach * (as opposed to iterators), we can normally only detect * interference after actions, not before. Further * CME-triggering checks apply to all other possible * violations of assumptions for example null or too-small * elementData array given its size(), that could only have * occurred due to interference. This allows the inner loop * of forEach to run without any further checks, and * simplifies lambda-resolution. While this does entail a * number of checks, note that in the common case of * list.stream().forEach(a), no checks or other computation * occur anywhere other than inside forEach itself. The other * less-often-used methods cannot take advantage of most of * these streamlinings. */
