-
-
Notifications
You must be signed in to change notification settings - Fork 245
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
Fix performance regression in bufferSliding #1217
Conversation
@@ -31,5 +31,8 @@ object compat { | |||
def hasDefiniteSize[X](i: IterableOnce[X]): Boolean = i.knownSize >= 0 | |||
|
|||
def newBuilder[From, A, C](bf: BuildFrom[From, A, C], from: From): mutable.Builder[A, C] = bf.newBuilder(from) | |||
|
|||
@inline def toSeq[A](array: Array[AnyRef]): Seq[A] = | |||
new scala.collection.immutable.ArraySeq.ofRef(array).asInstanceOf[Seq[A]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be hard to keep track of these internal helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas? Alternatively, it could be placed in reactive
(it has only one usage) but it would be even more scattered then.
I have put it in private [monix]
in case we can delete it one day.
Initially I wanted to point to the same ArraySeq
but mutable one doesn't perform well in my benchmarks. It seems like toSeq
is quite expensive there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to remember we have it I guess.
I'm running benchmarks again and the results are all over the place, I'll rerun it over the night with Slack/Chrome/etc. closed |
I've run it more times and it seems to be OK:
Still a lot of variance in some results between runs but there's consistent trend that it might be even slightly faster on Scala 2.13 now |
I think that's fine. You would help me if you merged this, given your 2.13 upgrade 🙂 |
I also updated to Scala 2.13.3 - sorry for the noise
2.12:
2.13, before the change:
2.13, after the change: