Optimise common operations on Array and List #5664
Conversation
Co-Authored-By: Jason Zaugg <jzaugg@gmail.com>
Specifically, the `slice` and `take` methods.
9bab358 to
52f7169
Compare
|
I suggest we either close the other PRs, merge this one, and add the benchmarks, or incorporate these commits into one of the other PRs and close this one. |
|
Ok, I wasn't clear whether that was the last word on that. I cherry-picked the commits, so then the benchmark is in here too. |
use Array block copy operations rather than builder/iterator
52f7169 to
eb5c513
Compare
|
Mima should be happy with these new commits. I haven't looked into whether each whitelist item is good to go. Leaving that up to @szeiger |
|
Looks good. |
|
Thanks, sounds like I can safely close the other PRs, since all their commits have been included here? Will get this one merged asap. |
|
Yes they can all be safely closed.
:)
… On 2 Feb 2017, at 18:33, Adriaan Moors ***@***.***> wrote:
Thanks, sounds like I can safely close the other PRs, since all their commits have been included here? Will get this one merged asap.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
bincompat-forward.whitelist.conf
Outdated
| }, | ||
| { | ||
| matchName="scala.collection.mutable.WrappedArray.sliceImpl" | ||
| problemName=DirectMissingMethodProblem |
There was a problem hiding this comment.
This is a breaking change. WrappedArrray is public and the new method is protected. As discussed before, all these changes need to be moved down into a private implementation class.
bincompat-forward.whitelist.conf
Outdated
| }, | ||
| { | ||
| matchName="scala.collection.mutable.WrappedArray.emptyImpl" | ||
| problemName=DirectMissingMethodProblem |
bincompat-forward.whitelist.conf
Outdated
| }, | ||
| { | ||
| matchName="scala.collection.mutable.WrappedArray.slice" | ||
| problemName=IncompatibleResultTypeProblem |
|
I'll try to fix these remaining issues after lunch. Should be ready in a few hours. |
We introduce a package-private superclass with the overridden implementations. This is public at the bytecode level, so it needs to be whitelisted.
|
@szeiger I merged your fixes for your own review -- thanks! |
follows up on scala#5664; fixes scala/scala-dev#301
|
Looks like this is causing LinkageErrors in user code. Should we revert, declare 2.11.9 a dud and cut 2.11.10? Detailed diagnosis by @xuwei-k:https://github.com/xuwei-k/Scala-2.11.9-FilteredTraversableInternal Explanation by @lrytz (hinted at by @sjrd)The reason why a reference to List(1).map(identity).filter(_ => true)In erasure type checking, |
|
@lrytz So we now have a situation that looks binary compatible in bytecode but leads to incompatibilities due to additional information in ScalaSig? |
|
it's not in the scala signature, but bytecode instructions (cast, call
receiver type). i'm not sure it's a binary incompatibility by our defnition
(no linkage error), as you get a ClassNotFound. but the end result is an
incompatibility we don't want to have in a minor release.
…On Mon, 3 Apr 2017 at 16:51 Stefan Zeiger ***@***.***> wrote:
@lrytz <https://github.com/lrytz> So we now have a situation that looks
binary compatible in bytecode but leads to incompatibilities due to
additional information in ScalaSig?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5664 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAHTVMdhEfsrDgq9snlldbMGqHMx1KPIks5rsQd_gaJpZM4Lwl1L>
.
|
|
oh, or maybe it's a NoClassDefFoundErro, which is a linkage error, i'm on
the phone :)
…On Mon, 3 Apr 2017 at 17:02 Lukas Rytz ***@***.***> wrote:
it's not in the scala signature, but bytecode instructions (cast, call
receiver type). i'm not sure it's a binary incompatibility by our defnition
(no linkage error), as you get a ClassNotFound. but the end result is an
incompatibility we don't want to have in a minor release.
On Mon, 3 Apr 2017 at 16:51 Stefan Zeiger ***@***.***>
wrote:
> @lrytz <https://github.com/lrytz> So we now have a situation that looks
> binary compatible in bytecode but leads to incompatibilities due to
> additional information in ScalaSig?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#5664 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAHTVMdhEfsrDgq9snlldbMGqHMx1KPIks5rsQd_gaJpZM4Lwl1L>
> .
>
|
|
"in the bytecode instructions" is the effect, not the cause. Binary compatibility only matters when you have separate compilation, in which case the compiler must be getting the idea of casting to (I'm only speculating here. I'd need more time to look into this but I don't expect to find any until after ScalaDays. For now the only safe solution appears to be never to add a new superclass, even if it's declared private and not supposed to leak.) |
My explanation was unclear, sorry: the return type of
I would say that MiMa did its job correctly: it identified the class being added, it was whitelisted. Any reference to a new class is a binary incompatibility.
The return type of |
|
Thanks, that makes it clearer. So we'd have to change this to cast to the actual type of the qualifier instead of the method's owner (which can be a supertype) to prevent these leaks?
I was hoping we could change MiMa to ignore new non-accessible superclasses and treat this case as binary compatible but with these owner casts leaking superclasses the only safe situation would be one where all methods implemented by the new superclass are overridden in all subclasses. |
This cannot easily be done, unfortunately. At the beginning of erasure, all types of trees are set to We could remember the type that the tree had after typer ( |
|
After further consideration
even this wouldn't be safe because it's not transitive. Adding a new superclass with overrides of all methods in all subclasses would be considered safe, adding a new override to an existing private class is safe, but the combination of both breaks binary compatibility. |
Revert #5664 because the binary incompatible change leaks via erasure
Consolidates the following PRs: