Hello! PS> I still disagree and pushing back on the support for splitting PS> after partial traversal. It’s not a pattern i think we should PS> encourage and propagate so such behaviour can be generally relied PS> upon, and predominantly for edge cases. That’s where the PS> complexity string is being pulled on. While your fix in isolation PS> is not terribly complex, it is more complex than the alternative PS> (which was actually the intent of the current impl, we just forget to include the check).
I still don't like doing this, but as Brian agreed with you [1], seems I have no other choice. Here's updated webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8148838/r3/ With best regards, Tagir Valeev. [1] https://twitter.com/BrianGoetz/status/694985109628387328 PS> Paul. >> On 3 Feb 2016, at 12:19, Tagir F. Valeev <amae...@gmail.com> wrote: >> >> Hello! >> >> Here's updated webrev: >> http://cr.openjdk.java.net/~tvaleev/webrev/8148838/r2/ >> >> PS> My inclination is to keep this simple and not support splitting after >> partial traversal. >> >> PS> Sometimes splitting after partial traversal might be more complex >> PS> not support (e.g. ArrayList). In other cases it’s more complex to >> PS> support and in such cases i would argue it is not worth it since >> PS> this kind of functionality is an edge case. >> >> I would still like that this problem is fixed (i.e. support splitting >> after advance, not just return null). This is probably an edge case >> for JDK, but might be crucial for library writers. As a library writer >> I actually had bad times working around this issue. While returning >> null instead of incorrect splitting would indeed be helpful, it will >> unnecessarily remove parallelization capabilities in some cases. For >> example, sometimes it's desired to extract the first element from the >> stream and create the stream from the tail. Returning null here would >> mean that the resulting stream will never split after that. >> >> To my opinion this fix is not very complex. It just adds several lines >> into single method (trySplit()). I added a comment which clarifies the >> intention. Other changes like extraction of initPusher() do not add >> complexity. Actually they reduce the complexity as four identical >> lambdas are merged into one. This patch actually reduces the size of >> created class files. With javac 9-ea+99 StreamSpliterators.java is >> compiled into 79004 bytes after my patch and 79472 bytes before my >> patch. Also this patch does not require primitive specializations and >> adds no overhead for common case when traversal is not started. >> >> PS> Testing wise you are right to be concerned about the increase in >> PS> test execution time. The lack of flatMap is definitely an >> PS> omission. In this case I think it is ok to replace map with >> PS> flatMap, as both result in stuff going into the holding buffer, >> PS> and we can use the smaller data sets, e.g. >> PS> "StreamTestData<Integer>.small”, that were recently added. >> >> I replaced all the datasets with .small alternatives (I think it's ok >> here as WrappingSpliterator behavior does not differ much for various >> sources) and removed all .map tests. Now the test works faster >> (like 25 sec -> 17 sec on by box). Please check. >> >> With best regards, >> Tagir Valeev. >> >> PS. >> >> PS> Paul. >> >>>> On 2 Feb 2016, at 09:24, Tagir F. Valeev <amae...@gmail.com> wrote: >>>> >>>> Please review and sponsor this fix: >>>> >>>> https://bugs.openjdk.java.net/browse/JDK-8148838 >>>> http://cr.openjdk.java.net/~tvaleev/webrev/8148838/r1/ >>>> >>>> When buffer traversal is already started, and split is requested, then >>>> the existing buffer should be carefully transferred to the prefix >>>> part. >>>> >>>> The only problem I see here is the testing time. Due to >>>> permuteFunctions() call it increases significantly what I added the >>>> flatMap() test. If this becomes an issue, I can write new simple test >>>> which tests flatMap() only (without permutations with other >>>> intermediate operations). >>>> >>>> With best regards, >>>> Tagir Valeev. >>>> >>
