Add NonEmptyList.zipWithIndex #1517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Current coverage is 92.40% (diff: 100%)@@ master #1517 diff @@
==========================================
Files 246 246
Lines 3732 3739 +7
Methods 3611 3618 +7
Messages 0 0
Branches 121 121
==========================================
+ Hits 3448 3455 +7
Misses 284 284
Partials 0 0
|
johnynek
reviewed
Jan 5, 2017
| */ | ||
| def zipWithIndex: NonEmptyList[(A, Int)] = { | ||
| @tailrec | ||
| def go(rest: List[A], ix: Int, acc: List[(A, Int)]): List[(A, Int)] = |
There was a problem hiding this comment.
while this is a nice implementation, I think ListBuilder can break the rules and mutate Lists and be faster. So, something like;
val bldr = List.newBuilder[(A, Int)]
var idx = 1
val it = tail.iterator
while (it.hasNext) {
bldr += (it.next, idx)
idx += 1
}
NonEmptyList((head, 0), bldr.result)will be faster, which is nice when we are talking about common libraries.
There was a problem hiding this comment.
Sure. I can update with your recommendations. For the record, here's the output of a quick benchmark I ran with the 2 different approaches.
[info] Benchmark (listSize) Mode Cnt Score Error Units
[info] NelZipWithIndexBench.zipWithIndexBuilder 1 thrpt 20 81866809.100 ± 1827269.422 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 10 thrpt 20 8397494.282 ± 265179.762 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 100 thrpt 20 837125.798 ± 19766.234 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 1000 thrpt 20 65129.596 ± 1794.893 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 10000 thrpt 20 6648.203 ± 103.004 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 100000 thrpt 20 540.067 ± 14.993 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 1000000 thrpt 20 16.204 ± 5.125 ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder 10000000 thrpt 20 0.205 ± 0.082 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 1 thrpt 20 88856828.471 ± 2776243.282 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 10 thrpt 20 2548195.555 ± 92940.797 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 100 thrpt 20 244793.012 ± 6162.114 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 1000 thrpt 20 21696.738 ± 632.243 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 10000 thrpt 20 4267.619 ± 805.001 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 100000 thrpt 20 356.116 ± 9.618 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 1000000 thrpt 20 10.396 ± 2.741 ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive 10000000 thrpt 20 0.111 ± 0.014 ops/s
There was a problem hiding this comment.
That's great! Thanks for benchmarking.
👍
There was a problem hiding this comment.
Way to be rigorous! So, if I'm reading this right, if the tail != Nil (listSize > 1) this is ~ 2-3x faster to use the builder. I would have assumed 2x or so because you skip the reverse.
|
👍 |
1 similar comment
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.