Skip to content

Review/fix definition of IArray #11298

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

Closed
odersky opened this issue Feb 3, 2021 · 14 comments · Fixed by #11354
Closed

Review/fix definition of IArray #11298

odersky opened this issue Feb 3, 2021 · 14 comments · Fixed by #11354
Milestone

Comments

@odersky
Copy link
Contributor

odersky commented Feb 3, 2021

IArray needs to be reviewed and tested before we can release it.

Right now, I see see several issues:

  • Missing methods, e.g. toList is not defined, but there are many others.
  • Inefficient implementation. Since IArray[A] is backed by Array[A] every access to
    the underlying array goes through reflection. We need to find a way to inline the access
    methods. This would be easier if we can expand opaque types before pickling since
    then inline methods and opaque types can co-exist.
  • Package structure. Should the full name be scala.opaques.IArray , or is something else better?
scala> val xs = IArray(1, 2, 3)                                                                                       
val xs: opaques.IArray[Int] = Array(1, 2, 3)

scala> xs.toList                                                                                                           
1 |xs.toList
  |^^^^^^^^^
  |value toList is not a member of opaques.IArray[Int]
@odersky odersky added this to the 3.0.0-RC1 milestone Feb 3, 2021
@propensive
Copy link
Contributor

I was planning on teaching IArray before Array on ScalaZONE, to fit with a general preference for immutability. So on the third point, my preference would be to have it available without an import.

@nicolasstucki
Copy link
Contributor

  • I believe we have implemented a large part of ArrayOps.
  • We should adapt immutable.ArraySeq and add an implicit conversion from IArray to it.

@nicolasstucki
Copy link
Contributor

Since IArray[A] is backed by Array[A] every access to the underlying array goes through reflection.

Actually, access to the array does not need reflection. It only has to load the module and call a method. Inlining would still help remove the module instantiation and method call.

Example 1
class E:
  def test1 =
    val a = Array(1)
    a(0)

  def test2 =
    val a = IArray(1)
    a(0)
public class E {
  public E();
    Code:
       0: aload_0
       1: invokespecial #14                 // Method java/lang/Object."<init>":()V
       4: return

  public int test1();
    Code:
       0: iconst_1
       1: newarray       int
       3: dup
       4: iconst_0
       5: iconst_1
       6: iastore
       7: astore_1
       8: aload_1
       9: iconst_0
      10: iaload
      11: ireturn

  public int test2();
    Code:
       0: iconst_1
       1: newarray       int
       3: dup
       4: iconst_0
       5: iconst_1
       6: iastore
       7: astore_1
       8: getstatic     #25                 // Field scala/opaques$arrayOps$.MODULE$:Lscala/opaques$arrayOps$;
      11: aload_1
      12: iconst_0
      13: invokevirtual #29                 // Method scala/opaques$arrayOps$.apply:([II)I
      16: ireturn
}
Example 2
class E:
  def test1 =
    val a = Array(1)
    a(0)

  def test2 =
    val a = IArray(1)
    a(0)

  extension (arr: IArray[Int]) def apply(n: Int): Int = arr.asInstanceOf[Array[Int]].apply(n)
public class E {
  public E();
    Code:
       0: aload_0
       1: invokespecial #9                  // Method java/lang/Object."<init>":()V
       4: return

  public int test1();
    Code:
       0: iconst_1
       1: newarray       int
       3: dup
       4: iconst_0
       5: iconst_1
       6: iastore
       7: astore_1
       8: aload_1
       9: iconst_0
      10: iaload
      11: ireturn

  public int test2();
    Code:
       0: iconst_1
       1: newarray       int
       3: dup
       4: iconst_0
       5: iconst_1
       6: iastore
       7: astore_1
       8: aload_0
       9: aload_1
      10: iconst_0
      11: invokevirtual #20                 // Method apply:([II)I
      14: ireturn

  public int apply(int[], int);
    Code:
       0: aload_1
       1: iload_2
       2: iaload
       3: ireturn
}

@odersky
Copy link
Contributor Author

odersky commented Feb 4, 2021

@nicolasstucki What about the other operations? They all go through genericArrayOps. Would that not slow things down?

@odersky
Copy link
Contributor Author

odersky commented Feb 4, 2021

@propensive There is a type alias in scala, so it is available without import. But it will print as opaques.IArray. One option would be to change the printer to avoid doing that. But that would mean special-casing IArray.

@odersky
Copy link
Contributor Author

odersky commented Feb 4, 2021

@nicolasstucki Yes, I think most missing methods come from ArraySeq. They are not implemented in ArrayOps.

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Feb 4, 2021

We implemented the operations of genericArrayOps as extensions to avoid the overhead. For Array all those operations also pass through ArrayOps.

We are missing ArraySeq in part because we either need to create a copy of a large part of collections, or we need to wait until we change modify it to be able to integrate IArray in it. We need to consider the binary compatibility of future versions.

@propensive
Copy link
Contributor

@odersky That's perfect then. My guess is that beginners wouldn't focus too much on the package name if they don't have to type it.

@sjrd
Copy link
Member

sjrd commented Feb 4, 2021

What is the purpose of putting it in a different package? Array is in the scala package. I see no reason why IArray should be anywhere else.

An if it should, scala.opaques is completely meaningless. It's like putting all classes in scala.classes and traits in scala.traits. No, if it should be somewhere else, it should be scala.collection.immutable.

@julienrf
Copy link
Contributor

julienrf commented Feb 4, 2021

I’m not a big fan of the I prefix, in IArray. It is not very explicit, it could as well stand for “Interface”… Would it be a problem to move IArray to scala.collection.immutable.Array? (We already have that for other collections like Iterable and scala.collection.immutable.Iterable)

Do we want to deprecate s.c.i.ArraySeq in favor of s.c.i.Array, at some point? I feel those two types overlap a lot, or is there a good reason to keep both?

@LPTK
Copy link
Contributor

LPTK commented Feb 4, 2021

@julienrf ArraySeq has the behavior of a proper Seq (toString, equals, hashCode) and can be used as a vararg, so it can't generally be replaced by a plain array.

@nicolasstucki
Copy link
Contributor

The missing methods can be added with #11319

@nicolasstucki
Copy link
Contributor

Package structure. Should the full name be scala.opaques.IArray, or is something else better?

#11329 seems to provide a good alternative.

@odersky
Copy link
Contributor Author

odersky commented Feb 8, 2021

It would be good to do some micro-benchmarks about performance of array operations vs IArray operations. Ideally the performance should be the same after warmup. We should test direct access with apply as well as common collection operations such as map, flatMap, filter, foldLeft. And we should test with elements of both primitive and reference types.

@nicolasstucki nicolasstucki removed this from the 3.0.0-RC1 milestone Feb 11, 2021
@nicolasstucki nicolasstucki linked a pull request Feb 11, 2021 that will close this issue
4 tasks
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants