Streamline the initialization path of Range.#11098
Conversation
Extract branches to be mostly dependent on `isInclusive` and `step >= 0`. These are often constant, and when they're not statically constant, they are probably predictable anyway. This commit upstreams scala-js/scala-js@a5337ed from Scala.js.
Previously, `Range` used a number of intermediate operations on `Long`s to avoid overflow. We can streamline a lot of code by using unsigned `Int` arithmetics. In particular, there is only 1 division in the initialization path, instead of 3. Although the fields have not changed, the content of `numRangeElements` is more strict for overfull ranges. This means that deserializing an overfull range from a previous version would not be safe. This is why we bump the SerialVersionUID. This commit upstreams scala-js/scala-js@d972218 from Scala.js.
|
I am pretty sure I was testing the different variants with Thyme, but I no longer have a recollection of what the different tests were. In general, the operations I see being used here are ones that I find to give pretty good performance, so it seems quite plausible that this would be a win. I don't have any specific advice about testing Range; I don't remember what I did, aside from the usual general strategy of picking tests where the new code is heavily stressed. I don't have the time at the moment. (I use JMH these days, usually launched with scala-cli if it's a small test just to validate my ideas about something being fast or not-slow.) |
lrytz
left a comment
There was a problem hiding this comment.
Thanks! Do we have or did you write on the Scala.js side some tests for the corner cases? LGTM, but it pretty subtle code :)
|
Existing tests seemed to be enough. Some relevant links:
|
|
Thank you - I only saw the junit test on a quick search. |
This PR upstreams scala-js/scala-js#5176 from Scala.js. We generally streamline the initialization paths of
Ranges by using unsignedIntarithmetics. When branches are needed, we make them depend onisInclusiveand/orstepif possible, since those are likely constant or predictable in practice.For Scala.js, these changes were really good, because
Longoperations are particularly expensive. Moreover, the inliner and constant-folder are such that common cases can fold away virtually all the computations of the new implementation. I would assume that the JVM is at least equally capable.I was hoping to find existing benchmarks for
Rangeintest/benchmarks, but there were none. @Ichoran, I guess you are, to this day, the most knowledgeable aboutRangeperformance, given 00d3f10. Any suggestion?