Hi Tagir, Test updates look good, thanks, that should reduce the test times on some of our test machines.
I still disagree and pushing back on the support for splitting after partial traversal. It’s not a pattern i think we should encourage and propagate so such behaviour can be generally relied upon, and predominantly for edge cases. That’s where the complexity string is being pulled on. While your fix in isolation is not terribly complex, it is more complex than the alternative (which was actually the intent of the current impl, we just forget to include the check). 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. >>> >
