scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Scala 2.13 inline optimizer unable to correctly look up `.head` method in `scala.collection.LinearSeq` #12787

Closed mdedetrich closed 1 year ago

mdedetrich commented 1 year ago

Reproduction steps

The current Scala 2.12/2.13 optimizer appears to be unable to correctly look up the .head method from the Scala scala.collection.LinearSeq when you happen to use @inline on a method that ends up transitively calling .head. The context of this is the current PR at https://github.com/apache/incubator-pekko/pull/305. If you checkout the branch and run sbt ++2.13.10 actor/compile you will get the following error messages

[error] /Users/mdedetrich/github/incubator-pekko/actor/src/main/scala-2.13/org/apache/pekko/util/ByteIterator.scala:370:50: org/apache/pekko/util/ByteIterator$MultiByteArrayIterator::org$apache$pekko$util$ByteIterator$MultiByteArrayIterator$$current()Lorg/apache/pekko/util/ByteIterator$ByteArrayIterator; is annotated @inline but could not be inlined:
[error] Failed to check if org/apache/pekko/util/ByteIterator$MultiByteArrayIterator::org$apache$pekko$util$ByteIterator$MultiByteArrayIterator$$current()Lorg/apache/pekko/util/ByteIterator$ByteArrayIterator; can be safely inlined to org/apache/pekko/util/ByteIterator$MultiByteArrayIterator without causing an IllegalAccessError. Checking instruction INVOKEINTERFACE scala/collection/LinearSeq.head ()Ljava/lang/Object; (itf) failed:
[error] The method head()Ljava/lang/Object; could not be found in the class scala/collection/LinearSeq or any of its parents.
[error]       getToArray(xs, offset, n, 1) { getByte } { current.getBytes(_, _, _) }
[error]                                                  ^
[error] /Users/mdedetrich/github/incubator-pekko/actor/src/main/scala-2.13/org/apache/pekko/util/ByteIterator.scala:385:63: org/apache/pekko/util/ByteIterator$MultiByteArrayIterator::org$apache$pekko$util$ByteIterator$MultiByteArrayIterator$$current()Lorg/apache/pekko/util/ByteIterator$ByteArrayIterator; is annotated @inline but could not be inlined:
[error] Failed to check if org/apache/pekko/util/ByteIterator$MultiByteArrayIterator::org$apache$pekko$util$ByteIterator$MultiByteArrayIterator$$current()Lorg/apache/pekko/util/ByteIterator$ByteArrayIterator; can be safely inlined to org/apache/pekko/util/ByteIterator$MultiByteArrayIterator without causing an IllegalAccessError. Checking instruction INVOKEINTERFACE scala/collection/LinearSeq.head ()Ljava/lang/Object; (itf) failed:
[error] The method head()Ljava/lang/Object; could not be found in the class scala/collection/LinearSeq or any of its parents.
[error]       getToArray(xs, offset, n, 8) { getDouble(byteOrder) } { current.getDoubles(_, _, _)(byteOrder) }
...

The current method is defined as

@inline private def current: ByteArrayIterator = iterators.head

More context can be found at https://contributors.scala-lang.org/t/scala-2-12-2-13-inliner-is-too-aggressive/6191. Will also try to create a minimised example

Problem

It appears that specifically for Scala 2.13, the optimizer has issues in inlining methods that happen to call .head from scala.collection.LinearSeq in the call chain. I suspect that this may be a result of the new collections included in Scala 2.13.

mdedetrich commented 1 year ago

@lrytz I have made a minimal reproduction at https://github.com/mdedetrich/scala-213-collections-inliner-bug .

scalway commented 1 year ago

Here is mdedetrich's example moved to scala-cli script:

// file:scala-213-collections-inliner-bug.scala

//> using scala "2.13.nightly"
//> using option "-opt-inline-from:<sources>" 
//> using option "-opt:l:inline"

import scala.collection.LinearSeq

object Main {
  val iterators: LinearSeq[Int] = Nil
  @inline def current: Int = iterators.head
  val asString = current.toString
}

to compile it for nightly 2.13 with optimiser warnings: scala-cli compile -Wopt:any-inline-failed 'scala-213-collections-inliner-bug.scala'

it produces warning (same as in sbt):

[warn] ./scala-213-collections-inliner-bug.scala:11:18
[warn] Main$::current()I is annotated @inline but could not be inlined:
[warn] Failed to check if Main$::current()I can be safely inlined to Main$ 
       without causing an IllegalAccessError. 
       Checking instruction INVOKEINTERFACE 
       scala/collection/LinearSeq.head ()Ljava/lang/Object; 
       (itf) failed:
[warn] The method head()Ljava/lang/Object; 
       could not be found in the class scala/collection/LinearSeq 
       or any of its parents.
[warn]   val asString = current.toString

to compile it for nightly 2.12 (-Wopt is not supported there): scala-cli compile --scala 2.12.nightly 'scala-213-collections-inliner-bug.scala' there is no warning in 2.12 whatsoever but it could be that such warnings are not implemented (as -Wopt is not supported)?

mdedetrich commented 1 year ago

@SethTisue Would it be out of the question to include this fix in the next upcoming Scala 2.13.11 RC/release?

EDIT: I just noticed that the bot already assigned it for Scala 2.13.12 so I guess its too late for that.

SethTisue commented 1 year ago

The bot behavior is just a default behavior that we can overrule, but I suggest that we let this one go until 2.13.12. Last-minute changes are tempting but risky.

som-snytt commented 1 year ago

Thanks, Seth. I might up late on Friday nite anyway, but I'd just as soon not be on call because I broke something.

mdedetrich commented 1 year ago

but I'd just as soon not be on call because I broke something.

I thought thats how we all enjoyed our Friday nights 😆

som-snytt commented 1 year ago

lrytz has pushed the PR into correcter territory.

mdedetrich commented 1 year ago

Since people love it when more work is pushed on them, what are the chances of this being backported into 2.12? This is on the presumption that the bug is a general one and was only shown in Scala 2.13 due to the collections rework (I could be wrong here).

If I scrap together some time I might be able to look into it a bit later if its not picked up, it would largely be shamelessly copying @som-snytt prior art anyways.

som-snytt commented 1 year ago

Oh, I see you do mention 2.12 in your OP comment, if not the title. Well, you'd have to open a ticket against 2.12, and then there would have to be a meeting of the SIP committee with a Scala Center community representative in attendance, and then some funding from a Lightbend subscriber. The prospects are promising. I bet the backport would be easy.

SethTisue commented 1 year ago

what are the chances of this being backported into 2.12

If a volunteer submits a backport, we'll review it.

som-snytt commented 1 year ago

The test case does not immediately reproduce for me on 2.12, although the code looks approximately the same. I think I'm using the old-style -opt options correctly. I'll at least push a test.

mdedetrich commented 1 year ago

Oh, I see you do mention 2.12 in your OP comment, if not the title. Well, you'd have to open a ticket against 2.12, and then there would have to be a meeting of the SIP committee with a Scala Center community representative in attendance, and then some funding from a Lightbend subscriber. The prospects are promising. I bet the backport would be easy.

It's not like the old days, eh?

som-snytt commented 1 year ago

@mdedetrich thanks for understanding me. To be clear to casual readers, as Seth said, we're in a kind of sweet spot in that Scala 2 may be winding down, but actually useful fixes of a certain scope are likely to be entertained and accommodated.

som-snytt commented 1 year ago

Sorry for the noise, the OP doesn't trigger on 2.12, because collections; the lrytz test triggers fine.

It is at https://github.com/scala/scala/pull/10415