Skip to content
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

Convert List.apply into :: chains #18567

Closed
wants to merge 2 commits into from
Closed

Convert List.apply into :: chains #18567

wants to merge 2 commits into from

Conversation

lolgab
Copy link
Contributor

@lolgab lolgab commented Sep 18, 2023

Reasoning:
Varargs in Scala use intermediate collections as shown in this example:

def foo = List("", "", "")
def bar = "" :: "" :: "" :: Nil

compiling with -Xprint:repeatableAnnotations gives:

def foo(): scala.collection.immutable.List =
  List().apply(scala.runtime.ScalaRunTime.wrapRefArray(["","","" : String]))
    .asInstanceOf[scala.collection.immutable.List]
def bar(): scala.collection.immutable.List = Nil().::("").::("").::("")

I want to avoid instantiating that extra Array and wrapping it into an ArraySeq to then create a List.

@filipwiech
Copy link

filipwiech commented Sep 18, 2023

Related issue: #17035. 👍

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Sep 19, 2023

This is less idiomatic in many cases. Do we have a noticeable performance gain?

Note that for large lists the bytecode will be larger which might make it harder for the JIT to optimize some code.

val pos = s.pos.sourcePos
(pos.line, pos.column)
}
val unsorted = sortedImp ++
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has quadratic complexity. Use ::: instead of ++.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to keep the code with the List.apply and implement the optimization in #17035.

@lolgab
Copy link
Contributor Author

lolgab commented Sep 19, 2023

It would be better to keep the code with the List.apply and implement the optimization in #17035.

Thank you for the review.
I wasn't aware of the WIP optimization for List.apply.

@lolgab lolgab closed this Sep 19, 2023
@lolgab lolgab deleted the avoid-list-apply branch September 19, 2023 10:16
@nicolasstucki
Copy link
Contributor

We should keep the changes from ++ to :::. Those do have a performance benefit.

@lolgab
Copy link
Contributor Author

lolgab commented Sep 19, 2023

We should keep the changes from ++ to :::. Those do have a performance benefit.

Opened another PR here: #18572

@SethTisue
Copy link
Member

SethTisue commented Sep 19, 2023

This has quadratic complexity

I don't think there is any complexity difference. Neither ++ nor ::: copies the right-hand list:

scala> val l1 = List(1, 2); val l2 = List(3, 4)
val l1: List[Int] = List(1, 2)
val l2: List[Int] = List(3, 4)
                                                                                                    
scala> (l1 ::: l2).drop(2) eq l2
val res0: Boolean = true
                                                                                                    
scala> (l1 ++ l2).drop(2) eq l2
val res1: Boolean = true

And they both copy the left-hand list — because they must.

See also Dale's remark on the new PR.

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 this pull request may close these issues.

4 participants