RFR-8148838: Stream.flatMap(...).spliterator() cannot properly split after tryAdvance()
Tagir F. Valeev
amaembo at gmail.com
Sat Feb 6 13:29:45 UTC 2016
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 , seems
I have no other choice. Here's updated webrev:
With best regards,
>> On 3 Feb 2016, at 12:19, Tagir F. Valeev <amaembo at gmail.com> wrote:
>> Here's updated webrev:
>> 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> Paul.
>>>> On 2 Feb 2016, at 09:24, Tagir F. Valeev <amaembo at gmail.com> wrote:
>>>> Please review and sponsor this fix:
>>>> When buffer traversal is already started, and split is requested, then
>>>> the existing buffer should be carefully transferred to the prefix
>>>> 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.
More information about the core-libs-dev