Array slice optimisations #5652
Conversation
f406f32 to
c356c6e
Compare
|
This looks like a good approach in principle but there seem to be issues with return types and presence or absence of a |
|
Hi
I thought so but apparently not – have fixed the code now
Please note the handling of Unit is different from the other cases as detailed in the comment
Otherwise there is a compile error
[error] C:\Users\dev\Desktop\code\scalac\scalac_perf\src\library\scala\collection\mutable\WrappedArray.scala:235: type mismatch;
[error] found : Array[Unit]
[error] required: Array[Unit]
[error] Note: Unit >: Unit, but class Array is invariant in type T.
[error] You may wish to investigate a wildcard type such as `_ >: Unit`. (SLS 3.2.10)
[error] new ofUnit(util.Arrays.copyOfRange[Unit](array, from, until))
[error]
So assuming that Array[Unit] always contains (java) null only
Should I change the path to needlessly fill Unit onto the result?
I will attach some benchmark results later
Prob next week will go through all of the PR and do this
Paid work gets in the way …..
Regards
Mike
From: Ichoran [mailto:notifications@github.com]
Sent: 18 January 2017 20:34
To: scala/scala <scala@noreply.github.com>
Cc: mkeskells <mike.skells@talk21.com>; Author <author@noreply.github.com>
Subject: Re: [scala/scala] Array slice optimisations (#5652)
This looks like a good approach in principle but there seem to be issues with return types and presence or absence of a ClassTag that makes me think it could not work as written. Have you compiled this locally?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#5652 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AVPl5W4cykxIXbz0SBFnnq-Rfnu-5HZiks5rTndIgaJpZM4LnSRG> .
|
| protected override def sliceImpl(from: Int, until: Int) = { | ||
| // cant use util.Arrays.copyOfRange[Unit](repr, from, until) - Unit is special and doesnt compile | ||
| // but Unit and null are analogous so values are not important to copy across | ||
| Array[Unit](until-from) |
There was a problem hiding this comment.
I think you missed the new. As written, this discards the value until-from to produce a single unit, then creates an array of length one. There should be tests of all the slices for all the different implementations to catch stuff like this!
There was a problem hiding this comment.
If you wanted to actually copy the array (to preserve whether the contents are null or instances of scala.runtime.BoxedUnit--perversely, either one is okay--you could
java.util.Arrays.copyOfRange(
repr.asInstanceOf[Array[scala.runtime.BoxedUnit]],
from,
until
).asInstanceOf[Array[Unit]]
(maybe on a single line--I'm just spreading it out here to make it easier to see).
| // cant use | ||
| // new ofUnit(util.Arrays.copyOfRange[Unit](array, from, until)) - Unit is special and doesnt compile | ||
| // but Unit and null are analogous so values are not important to copy across | ||
| new ofUnit(Array[Unit](until-from)) |
There was a problem hiding this comment.
Same mistake here as with the ArrayOps version.
|
This looks pretty good! Benchmarks for this one are less important, as it is super-obvious that this will be a big win in many cases. However, unit tests are very important to make sure that now, and in the future, no indices have been messed up. |
|
HiI had assumed that the existing unit tests for the collections api would cover itIf not then I presume that we should pr a set of tests
Several of these products that we have and have not yet submitted are collected turn issueso and tweaks
What is the expectation for exception behaviour it is isn't in the scala docEg myseq(-1)Is it documented elsewhereRegardsMike
Sent from Yahoo Mail on Android
On Thu, 19 Jan 2017 at 1:39, Ichoran<notifications@github.com> wrote:
This looks pretty good! Benchmarks for this one are less important, as it is super-obvious that this will be a big win in many cases. However, unit tests are very important to make sure that now, and in the future, no indices have been messed up.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
You should verify that a proper unit test exists, or write a new one. You can't automatically assume that when you split one implementation into ten that the unit tests will properly cover all ten cases. When everything shared an implementation, a single unit test with a single data type would have been enough. I'm not sure understand what you were asking about "expectation for exception behavior". |
|
That was what I meant
I have added some initial unit test. I could not see any existing test to extend
Please can you review these tests to see if they look right
I am not convinced that the Array[Unit] test will work
Not sure how to check that BoxedUnit vs null in the arrays though, without resorting to java!
Tests pass locally against 2.11.8 without my mods. Ran out of time to test with the mods, but thought that it was better to give some sight of these and see if they were in the right direction/sufficient etc
|
|
I will remove the isLenient etc checks in next push
|
|
Looks very extensive! That's good. I can't eyeball whether you've successfully gotten around generic code making everything look like Object, but it looks like you probably have. (It would be more idiomatic to use ClassTag, but I think it's okay anyway.) I don't have time right now to think about binary compatibility (which is why the test failed). That could be a problem. Maybe someone else can pick this up? @szeiger ? |
These look harmless and could simply be whitelisted. They are members of final classes, so the methods are effectively final anyway. MiMa shouldn't report them in the first place. These changes break binary compatibility in a fundamental way. You can never add a new abstract method or make a non-final method final. For 2.11 & 2.12 you could do these changes in a new private implementation class inside the |
|
I have updated the patch to, hopefully, comply with binary compatibility needs I have taken the liberty of marking WrappedArray as @deprecatedInheritance like ArrayOps. Happy to remove this if they should not be in step with ArrayOps, but it seemed sane to me What is the appropriate way to provide a markup for the changes at 2.13 as proposed above? For the moment I have just added comments //at 2.13 .... I have also moved the regression test to scala.collections, and added some support ofor other classes that follow a similar pattern |
Typically we don't introduce deprrcations in minor releases, so I'd leave this until 2.13.x.
You can just keep the extra commit that will eventually target 2.13.x on a separate branch in your repository and link to it from here.
You can add the test for existing functionality in the first commit of this PR, then change the implementation in the second. Our pull request validation checks that each commit passes the test suite. |
|
HiWhen we can agree that the problem contains the correct set of changes to the runtime and the tests I can either remove all the commits and apply in this order or generate a new pr with just these commitsMike
Sent from Yahoo Mail on Android
On Tue, 24 Jan 2017 at 23:06, Jason Zaugg<notifications@github.com> wrote:
It seems sensible to be that this regression test should be the subject of a separate PR, otherwise it proves nothing, just that the code with my chnages matches some new test
You can add the test for existing functionality in the first commit of this PR, then change the implementation in the second. Our pull request validation checks that each commit passes the test suite.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
The changes for binary compatibility are not enough. Adding a protected concrete trait method is not even backward compatible in 2.11 (but it is in 2.12). It is not forward compatible in either version. You really need to move all of that stuff down into a private implementation class and keep the trait as it is. |
|
/rebuild |
|
The test failure is likely due to your branch being based on an old version of 2.11.x. To get up to date, could you do a |
…edSeq implementations
…y operations rather than builder/iterator for slace and take
|
applied the git foo suggested by @adriaanm |
ff99d80 to
51a3f22
Compare
|
Looks good. Please add the remaining problem filters from the log to |
|
Is there anything that I need to do here? |
|
The filters are part of the code tree: https://github.com/scala/scala/blob/2.11.x/bincompat-backward.whitelist.conf the items to add to |
|
See #5664 for a consolidated version with test & mima fixes |
|
Commits moved to #5664. Thanks! |
No description provided.