Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Chunk.traverse take 2 #1957

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

johnynek
Copy link
Contributor

this is a followup to #1952

In that, I used a binary tree to implement Chunk.traverse. This gives stack safety, but it doesn't make it faster. I observed later and noted in typelevel/cats#3517 that nothing requires the tree to be binary.

So, here, I use a tree with 128 children, and in all the internal nodes, I use map2Eval which allows short-circuiting and laziness for applicative instances that can avoid evaluating one side in some cases.

This results in a significant speed-up over the foldRight based implementation:

[info] Benchmark                       (chunkCount)  (chunkSize)   Mode  Cnt        Score       Error  Units
[info] ChunkBenchmark.traverse                    1           16  thrpt    6  1694967.533 ± 28309.177  ops/s
[info] ChunkBenchmark.traverse                    1          256  thrpt    6    45790.706 ±  1118.671  ops/s
[info] ChunkBenchmark.traverse                    1         4096  thrpt    6    38639.014 ± 12449.586  ops/s
[info] ChunkBenchmark.traverse                    1       409600  thrpt    6    16266.069 ±   512.302  ops/s
[info] ChunkBenchmark.traverseViaFold             1           16  thrpt    6   830798.147 ± 95479.003  ops/s
[info] ChunkBenchmark.traverseViaFold             1          256  thrpt    6    40813.175 ±  1310.140  ops/s

note, fold failed with stack overflow for 4096 and 409600 so I couldn't compare them.

@@ -627,7 +627,17 @@ object Chunk extends CollectorK[Chunk] with ChunkCompanionPlatform {

/** Creates a chunk backed by a `Chain`. */
def chain[O](c: Chain[O]): Chunk[O] =
seq(c.toList)
if (c.isEmpty) empty
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the generic Iterable implementation and seems strictly better than converting to List first, which is what we were doing before.

@SystemFw SystemFw merged commit d62699a into typelevel:main Jul 14, 2020
@mpilquist mpilquist added this to the 2.4.3 milestone Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants