Update to 2.13.0 #248
Update to 2.13.0 #248
Conversation
| @@ -30,7 +31,7 @@ private[util] class BatchExecutor[In, Out]( | |||
| sizeThreshold: Int, | |||
| timeThreshold: Duration = Duration.Top, | |||
| sizePercentile: => Float = 1.0f, | |||
| f: Seq[In] => Future[Seq[Out]] | |||
| f: Iterable[In] => Future[Seq[Out]] | |||
There was a problem hiding this comment.
This raises, rather than lowers the requirements. Still, this seems reasonable in the name of unbenchmarked performance.
util-core/src/test/scala/com/twitter/concurrent/AsyncStreamTest.scala
Outdated
Show resolved
Hide resolved
|
More errors than I thought on different cross-versions. Closing for now, until there is something closer. |
| * builder. A value containing the current version of the collection | ||
| * is notified for each incoming event. | ||
| */ | ||
| def build[U >: T, That <: Seq[U]](implicit factory: Factory[U, That]): Event[That] = buildAny(factory) |
There was a problem hiding this comment.
add a seq-specific one, so that build that picks up any ambient factory/cbf builds at least a Seq. If we don't do this, on 2.13.0-RC1, we'll pick up a Factory[T, Array[T]] from the companion object of Factory, and build an Event[Array[T]] which is not a subtype of Event[Seq[T]]
| def register(s: Witness[That]): Closable = { | ||
| val b = cbf() | ||
| val b = factory.newBuilder |
There was a problem hiding this comment.
The contract of builder makes calling result() more than once undefined behaviour (with the exception of ReusableBuilder which can call result() again after calling clear, but we're not clearing here anyway). That means that this relies upon (and always has relied upon) builder being well-behaved in cases where it doesn't have to play that nice according to its contract.
| browseDir(f, loader, "", buf) | ||
| else | ||
| browseJar(f, loader, buf, history) | ||
| if (!history.contains(uri)) { |
There was a problem hiding this comment.
Done URI checking local to this function -- forking in test resulted in duplicate CpInfo's in the result
util-core/src/main/scala/com/twitter/concurrent/AsyncStream.scala
Outdated
Show resolved
Hide resolved
| @@ -157,8 +157,8 @@ object Reader { | |||
| require(chunkSize > 0, s"chunkSize should be > 0 but was $chunkSize") | |||
|
|
|||
| @tailrec | |||
| private def loop(acc: Seq[Buf], in: Buf): Seq[Buf] = { | |||
| if (in.length < chunkSize) acc :+ in | |||
| private def loop(acc: ListBuffer[Buf], in: Buf): Seq[Buf] = { | |||
There was a problem hiding this comment.
this isn't getting any prettier. I'd be happy to replace with a non-mutable loop, but went for minimal invasiveness
util-core/src/test/java/com/twitter/concurrent/SpoolCompilationTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| test("close while read pending") { | ||
| def closeWhileReadPending = { |
There was a problem hiding this comment.
Some of these had to be factored out, for reasons that aren't clear to me, and may be bugs in assert or scalac: it seems to have to do with macros re-evaluating/re-capturing variables that trip up scalac.
There was a problem hiding this comment.
reported at scala/bug#11556
There was a problem hiding this comment.
I ran into issues with PipeTest also in my previous minor experiment, subscribed to the bug.
There was a problem hiding this comment.
Factoring the test body into a method consistently worked around for now, if you run into it elsewhere.
| .toSeq | ||
| .sortWith(_._1 < _._1) | ||
| .map { case (k, v) => BucketAndCount(k, k + 1, v) } | ||
| .sortBy(_.lowerLimit) |
There was a problem hiding this comment.
simplified after eliminating the deprecated mapValues
util-stats/src/test/java/com/twitter/finagle/stats/StatsReceiverCompilationTest.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## develop #248 +/- ##
===========================================
+ Coverage 46.68% 46.97% +0.29%
===========================================
Files 231 229 -2
Lines 14848 14752 -96
Branches 1187 918 -269
===========================================
- Hits 6932 6930 -2
+ Misses 7916 7822 -94
Continue to review full report at Codecov.
|
| @@ -253,4 +256,22 @@ trait StatsReceiver { | |||
| } | |||
| } | |||
|
|
|||
| abstract class AbstractStatsReceiver extends StatsReceiver | |||
| abstract class AbstractStatsReceiver extends StatsReceiver { | |||
There was a problem hiding this comment.
In order to make overriding varargs from Java source compatible between 2.12.x and 2.13.x, made the varargs methods final, and made a non-varargs non-aliased abstract method with a scala.collection.Seq argument as an extension point for overriding.
martijnhoekstra
changed the title
martijnhoekstra
changed the title
martijnhoekstra
force-pushed
the
rebase
branch
2 times, most recently
from
da7c02e
to
df32eb8
Compare
Yes, I am pulling in this branch and start working on the internal build as well as benchmarks. |
|
Thats really good news. I'll quit any further crazy rebases then, at least
until you have more to share on that front.
…On Tue, Jun 25, 2019, 22:22 Yufan Gong ***@***.***> wrote:
That benchmark run will be complete in 10.5 days.
I can't spare hardware to chug on this for that amount of time. I'll try
to isolate the likeliest things to have regressed, and run limited runs on
those, but that's going to be very limited.
Can you see if hardware can be allocated from your side to chug through
the bench suite for all benchmarks for all 5 configurations?
Yes, I am pulling in this branch and start working on the internal build
as well as benchmarks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#248?email_source=notifications&email_token=AAGXOEM2CLZBSMZ6O7JWHMLP4J5BLA5CNFSM4HGARWM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYRPLJY#issuecomment-505607591>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGXOEPKIAXYD3FEUTC3UKDP4J5BLANCNFSM4HGARWMQ>
.
|
|
Updates: We will run a few more benchmarks in addition to the ones you have run. Thanks a ton for doing this! |
|
Interpreted from https://gist.github.com/martijnhoekstra/3303264b113a430cf762b7d3d3ab0d4c: util-benchmarks:concurrent
finagle
hashing
io
jvm
string
util
|
|
The Cumulative Gauge benchmarks were looking good now. The 3 AsyncStream ones had too much jitter in the run I did to be of value. Can you include them on your run? Note that the benchmarks I ran in many cases didn't have the runtime nor the number of warmup and measurement iterations recommended in the tests, simply for the sake of not taking days. I basically cut every corner that could be cut without crashing or burning. I would still have expected regressions to at least show up, but possibly evaporate under more scrutiny, but more principled runs for everything are welcome. Also, I left 2.11 out of the benchmarks and treated 2.12 pre PR as a baseline to measure 2.12 post PR and 2.13 post PR against. That may actually be an acceptable corner to cut. |
| @@ -1404,7 +1422,7 @@ def join[%s](%s): Future[(%s)] = join(Seq(%s)).map { _ => (%s) }""".format( | |||
| sizeThreshold: Int, | |||
| timeThreshold: Duration = Duration.Top, | |||
| sizePercentile: => Float = 1.0f | |||
| )(f: Seq[In] => Future[Seq[Out]] | |||
| )(f: Iterable[In] => Future[Seq[Out]] | |||
There was a problem hiding this comment.
This is a breaking change, just want to make sure this is necessary. If so we should add an entry to CHANGELOG(i can add it on my branch).
There was a problem hiding this comment.
Maybe it's better to keep this at scala.collection.Seq then.
- For 2.13 it may be a breaking change, but some refactoring is already expected there.
- For 2.12.x it won't break anything.
I will investigate, and come back to it this Tuesday, as I'm heading into a long weekend.
There was a problem hiding this comment.
b5b124b is a way to do it without the Iterable
There was a problem hiding this comment.
The tests seem to be mocking me over that change.
I've never used any mocking frameworks. What's the correct way to fix this? Include function composition in the mock?
There was a problem hiding this comment.
Hey @martijnhoekstra, I modified tests to mock the composed method, and verify against m.
val m = mock[Any => Future[Seq[Int]]]
val f = mock[scala.collection.Seq[Int] => Future[Seq[Int]]]
when(f.compose(any())).thenReturn(m)
I have your branch internally almost ready, passed all tests but only running the rest of benchmarks.
| @@ -126,7 +126,7 @@ class StorageUnit(val bytes: Long) extends Ordered[StorageUnit] { | |||
| def max(other: StorageUnit): StorageUnit = | |||
| if (this > other) this else other | |||
|
|
|||
| override def toString(): String = inBytes + ".bytes" | |||
| override def toString(): String = s"$inBytes bytes" | |||
There was a problem hiding this comment.
I need to revert this for some utility usage.
There was a problem hiding this comment.
for the format, or for the interpolator? I overlooked the original ., that change was unintentional, but the interpolator is there because any2stringadd is on its way out, and should be either s"$inBytes.bytes" with, or inBytes.toString + ".bytes" without interpolator
There was a problem hiding this comment.
gotcha, I will only add the . then.
| final override def stat(verbosity: Verbosity, name: String*): Stat = statImpl(verbosity, name) | ||
| protected def statImpl(verbosity: Verbosity, name: scala.collection.Seq[String]): Stat | ||
|
|
||
| final override def addGauge(name: String*)(f: => Float): Gauge = addGauge(Verbosity.Default, name: _*)(f) |
There was a problem hiding this comment.
We should add @varargs for these two addGauge methods, right?
There was a problem hiding this comment.
@varargs makes little sense if the last argument list isn't the varargs list. Shall I check if they actually generate any different bytecode?
There was a problem hiding this comment.
It does produce different bytecode.
The code
object Test {
@varargs def withAnnotation(args: Int*)(extra: String) = args.length
}
produces extra an extra forwarder method:
public int withAnnotation(int..., java.lang.String);
descriptor: ([ILjava/lang/String;)I
flags: ACC_PUBLIC, ACC_VARARGS
Code:
stack=3, locals=3, args_size=3
0: aload_0
1: getstatic #31 // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
4: aload_1
5: invokevirtual #35 // Method scala/runtime/ScalaRunTime$.wrapIntArray:([I)Lscala/collection/immutable/ArraySeq;
8: aload_2
9: invokevirtual #38 // Method withAnnotation:(Lscala/collection/immutable/Seq;Ljava/lang/String;)I
12: ireturn
But this can't ever be called with varargs from Java, since while this is valid JVM bytecode, there is no equivalent Java code, where the varargs must always be last.
In fact, I'd argue that the annotation here can't be useful, and should be deprecated in scala
| @@ -12,17 +12,17 @@ val zkDependency = "org.apache.zookeeper" % "zookeeper" % zkVersion excludeAll( | |||
| ExclusionRule("javax.jms", "jms") | |||
| ) | |||
| val slf4jVersion = "1.7.21" | |||
| val jacksonVersion = "2.9.8" | |||
| val jacksonVersion = "2.9.9" | |||
There was a problem hiding this comment.
Is this a necessary change along with this diff? or we can bump up the version separately?
There was a problem hiding this comment.
There is no 2.9.8 for 2.13.0, so we have to bump it independently, we have to bump it before and rebase this on top of that.
I have a branch where that's done --develop...martijnhoekstra:pre213 is a diff of a reasonable bump of dependencies and elimination of deprecations that go along with that. But to be honest, I'm not sure what value that brings, other than better baseline for benchmarking maybe? I doubt it would matter much.
|
@yufangong Do you have an update how benchmarks are going? Still ongoing? Are there already any provisional "red" results I can start working on? |
|
I am only running against 2.12, no obvious red flag so far |
|
@yufangong I snuck sbt 1.3.0 RC3 in in 85fb0fd because RC2 has some annoying bugs. I'm actually not too sure if sbt 1.3 should be part of this PR at all, or be factored into a different PR entirely (when 1.3.0 is fully released). What's the status on your side? |
|
@martijnhoekstra agreed that we should make it against a released version, I changed it to 1.2.8 and it seems working. Apologies that I was not actively working on this past week but going to get back on this branch next week. |
|
@yufangong what's the current status on benchmarking and review? Is this good to go if the benches are good? |
We have just upgraded Jackson version in source as part of the util cross-build effort. The most recent benchmark BrokerBenchmark seems a bit regression. I'll run again to see if thats valid. |
| @@ -313,7 +313,7 @@ trait App extends Closable with CloseAwaitably { | |||
| deadline: Time | |||
| ): Future[Unit] = { | |||
| Future | |||
| .collectToTry(lastExits.asScala.toSeq.map(_.close(deadline))) | |||
| .collectToTry(lastExits.asScala.map(_.close(deadline)).toSeq) | |||
There was a problem hiding this comment.
why does it matter where the toSeq is?
There was a problem hiding this comment.
I think it was part of a refactor that turned out wasn't needed, and this is a vestigial remainder of that refactor.
In other words, I don't think it matters, and pushed a commit to undo it
|
benchmarks are all clear, feel free to take a look at the result see if anything jumps out |
could/should be squased onto 1af964a
|
@martijnhoekstra It is merged here d5d20cc! Yay! Thanks for your patience! Can you share us your strategy of this migration that we can pass to finagle/finatra/etc as well as the internal source repo? Are there any automation tool or migration tool used and helpful? Thanks again! |
|
This migration, I did with elbow grease: set the scala version, and fix all deprecation warnings and compiler errors. For other repos, I'd make sure that you update all dependencies to a version that cross-builds, fix all deprecation warnings, then run the scalafix at https://github.com/scala/scala-collection-compat#collection213crosscompat, and take that as a starting point -- does the generated code make sense, or does it need an extra layer of shine. For example, in this repo, in many cases an I intend to work my way towards Chill on 2.13 migration. |
|
@martijnhoekstra Thanks so much! Closing this PR as resolved. |
|
ticket about publishing for 2.13: #253 |
Aim for (but not always accomplish) source compatibility
Prefer to use "default"
Seqoverscala.collection.SeqProblem
Explain the context and why you're making that change. What is the
problem you're trying to solve? In some cases there is not a problem
and this can be thought of being the motivation for your change.
Solution
Align with 2.13.0 collections
Result
Building under 2.13.0