Optimizations for ZIO#foreach* methods
#9365
Conversation
|
Aghh, of course Scala 2.12 doesn't have some of the methods I'm using in this PR for ArraySeq. I'll set to draft while I fix it |
| case Some(n) => foreachPar(n)(as)(f) | ||
| case None => foreachParUnbounded(as)(f) | ||
| } | ||
| as.size match { |
There was a problem hiding this comment.
Wouldn't it be better to use as.knownSize? Iterating a List will be slow due to memory locality, not just because of the element traversal.
There was a problem hiding this comment.
There are other things that depend on having the size of the collection, such as the sizing of an array used within one of the internal methods. So using knownSize won't work because it will return -1 for Lists or other collections that their size is not known. At least with this PR we're ensuring that size is called just once, unlike before that it was being called 2 or 3 times
| ZIO.parallelismWith { | ||
| case Some(n) => foreachParDiscard(n)(as)(f) | ||
| case None => foreachParUnboundedDiscard(as)(f) | ||
| ZIO.succeed(as).flatMap { as => |
There was a problem hiding this comment.
Is there a reason you need ZIO.succeed + flatMap here vs ZIO.suspendSucceed?
There was a problem hiding this comment.
I just wanted to avoid accidentally using as (the lazy => Iterable[A]) as method arguments, but I think suspendSucceed is probably cleaner so I'll change this
|
|
||
| final protected def fromArraySeq[A](seq: mutable.ArraySeq[A]): Chunk[A] = { | ||
| val arr = seq.array | ||
| Chunk.fromArray(Array.copyAs(arr, arr.length)(seq.elemTag)).asInstanceOf[Chunk[A]] |
There was a problem hiding this comment.
I'm very surprised the fromArray method doesn't copy/require Unsafe...
There was a problem hiding this comment.
Yeah the Unsafe implicit would actually be good to have. However that wouldn't be binary compatible :(
| val iterator = map.iterator | ||
| val builder = Map.newBuilder[Key2, Value2] | ||
|
|
||
| val ft = f.tupled |
There was a problem hiding this comment.
Is this better than unpacking the tuples manually?
There was a problem hiding this comment.
Just thought it makes the code cleaner. Changed to manual unpacking as it's more performant
| arr(i) = b | ||
| i += 1 | ||
| } | ||
| .as(arr) |
There was a problem hiding this comment.
Do we have an as operator that doesn't suspend the value?
| .as(arr) | |
| .flatMap(_ => Exit.succeed(arr)) |
There was a problem hiding this comment.
No, I think your suggestion is the best that we can do
There was a problem hiding this comment.
Although it wouldn't work without some sort of suspension as arr would otherwise be read before the whileLoop is evaluated
There was a problem hiding this comment.
On second thought, I'm going to keep this as is for now and revisit this in a separate PR as there are multiple places that this is occuring
This PR contains a number of micro-optimizations to improve performance of
ZIO.foreach*andZIO.foreachParmethods.The optimizations are mostly focused on avoiding conversion of abstract
Iterables (which use a List as the concrete type) to specialized collections (such as Arrays and Maps), and avoiding calculating a collection'ssizemultiple times inforeachParmethods (which makes things even worse when the abstract Iterable is a list).There are 2 optimization which contribute to an improvement of performance to
foreach*methods but will likely improve performance elsewhere:mutable.ArraySeq[A](which is what arrays are wrapped in when they're casted toIterables) inChunk.fromIterable, so that we can simply copy the underlying array instead of iterating and adding elements to a builder.strategy.unsafeOnQueueEmptySpacein various Queuetake*/pollmethods when the queue was already empty