RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Paul Sandoz]

> On 18 Apr 2016, at 14:35, Tagir F. Valeev <amaembo at gmail.com> wrote:
> 
> Hello!
> 
> Thank you for review!
> 
> PS>  913         UnorderedSliceSpliterator(T_SPLITR s, long skip, long limit) {
> PS>  914             this.s = s;
> PS>  915             this.unlimited = limit < 0;
> PS>  916             this.skipThreshold = limit >= 0 ? limit : 0;
> PS>  917             this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
> PS>  918                 (skip + limit) /
> PS> ForkJoinPool.getCommonPoolParallelism() + 1) : CHUNK_SIZE;
> PS>  919             this.permits = new AtomicLong(limit >= 0 ? skip + limit : skip);
> PS>  920         }
> PS>  921
> 
> PS> Note the common pool parallelism can never be 0. I dunno if you
> PS> added 1 for that or another reason.
> 
> It's actually
> ((skip + limit) / ForkJoinPool.getCommonPoolParallelism()) + 1
> Not
> (skip + limit) / (ForkJoinPool.getCommonPoolParallelism() + 1)
> 
> Probably  I should add explicit parentheses to make this clear. One is
> added exactly to make chunkSize at least 1.
> 

Doh. I think i need glasses…

A comment on the range might help too.


> PS> Did you consider:
> 
> PS>   (skip + limit) / AbstractTask.LEAF_TARGET
> 
> PS> ?
> 
> It should not make drastic changes in my test, but I will try.
> 

Ok.


> PS> What if chunkSize is zero? should it be a minimum of 1?
> 
> PS> Testing wise i think our existing tests cover things ok.
> 
> PS> Performance-wise looks good. Probable primes are my favourite way
> PS> of easily increasing Q (cost per op) :-)
> 
> PS> Can you run the stream tests and the perf tests with parallelism disabled:
> 
> PS>   -Djava.util.concurrent.ForkJoinPool.common.parallelism=1
> 
> Ok. I think I should also test the performance for some high-N low-Q
> task to check whether it not degrades. Will perform all the tests
> later this week.
> 
> By the way is these some special place to commit/store JMH tests
> (except CodeReview server), so they could be reused later?
> 

Not that i am aware of. The best thing for the moment is to place ‘em on cr.openjdk as you have done.

Paul.