-opt:l:inline breaks incremental compilation

[eed3si9n]

This was originally reported by @eparejatobes as https://github.com/sbt/sbt/issues/4125#issuecomment-385992117

Copying here the relevant portion of the link above; see ohnosequences/sbt4125 for more

steps

scalacOptions ++= Seq(
  "-unchecked",
  "-opt-warnings:_",
  "-opt:l:inline",
  "-opt-inline-from:**"
)

Open a shell and

git checkout "x=1"

Now run sbt on a different shell and do ~test. Once it runs for the first time,

git checkout "x=2"

Take a look at the test passing, and the definitions of x and y.

problem

Incremental compilation when used together with the optimizer generates incorrect bytecode: modifications in methods/vals are not propagated to wherever they are being inlined.

expectation

The incremental compiler should not generate incorrect bytecode.

I'd say we revise the expectation to be that change to x=2 is reflected in compilation and run.

notes

I am not sure how prevalent these optimization flags are, but we should probably print warning or invalidate all sources, similar to 50% threshold mechanism.

[smarter]

Basically, if a method is inlineable, its body is part of its API signature. We deal with that in dotty by storing the pretty-printed body of the method in a signature in ExtractAPI, though it'd be better to store a hash: https://github.com/lampepfl/dotty/blob/a3c4ec793545fd2b922eb469f7fc4d4a7d26b05f/compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala#L593-L602

[eed3si9n]

Interesting. That would give better usability compared to invalidating all sources :)

[eparejatobes]

Basically, if a method is inlineable, its body is part of its API signature (...)

What about def f = { g(0): @inline }?

[smarter]

Does that actually do anything in Scala 2.12 ?

[eparejatobes]

Does that actually do anything in Scala 2.12 ?

per the docs it should, and I just checked it in the repo linked above; javap yields

with call-site @inline

public int z();
    Code:
       0: iconst_1
       1: getstatic     #19                 // Field issues/sbt4125/inlineFunTimes$.MODULE$:Lissues/sbt4125/inlineFunTimes$;
       4: ifnonnull     9
       7: aconst_null
       8: athrow
       9: iconst_1
      10: iadd
      11: ireturn

no @inline

  public int z();
    Code:
       0: iconst_1
       1: getstatic     #19                 // Field issues/sbt4125/inlineFunTimes$.MODULE$:Lissues/sbt4125/inlineFunTimes$;
       4: invokevirtual #22                 // Method issues/sbt4125/inlineFunTimes$.x:()I
       7: iadd
       8: ireturn

[smarter]

Huh. I would vote to deprecate that feature then :P

[eed3si9n]

Could we deprecate all inlining?

[smarter]

It can be pretty important for performance

[eed3si9n]

http://slides.com/cb372/inline-specialized-berlin-2016#/27 says it's slightly slower when used with HotSpot, in that particular benchmark.

[eed3si9n]

For now, we should at least print warnings if it's anything less than 100% compilation.

To potential contributor. Related source might be https://github.com/sbt/zinc/blob/v1.1.7/internal/zinc-core/src/main/scala/sbt/internal/inc/IncrementalCommon.scala#L125-L132

[jvican]

Uf, I haven't looked at the details but fixing this it's quite complicated. We would need to teach Zinc:

1. To predict inlining that happens in genbcode (Zinc happens after pickler, bytecode optimizations happen at the end of the pipeling)
2. To understand the semantics of inlining (they can be complicated, not an easy task given how many heuristics they are involved)

I vote to:

1. Disable incremental compilation when inlining is involved.
2. Notify the user of this behaviour and advise him to not enable inlining by default in their build.

I'm optimistically labelling this wontfix.

[smarter]

I still think it makes sense for the scalac bridge to implement what I said in https://github.com/sbt/zinc/issues/537#issuecomment-385999145

[jvican]

I still think it makes sense for the scalac bridge to implement what I said in #537 (comment)

Missed that. Can the scalac genbcode ever inline something that is not marked as @inline? If it can't, then I like your solution. This isn't a high priority for me, so help on this is welcome.

[smarter]

Can the scalac genbcode ever inline something that is not marked as @inline?

Yes with call-site @inline, as mentioned in https://github.com/sbt/zinc/issues/537#issuecomment-386005581 and I also still stand by https://github.com/sbt/zinc/issues/537#issuecomment-386006868, this is a terrible feature that should be removed :).

[eed3si9n]

Let's start the petition for the call-site inline removal. Where do we start?

[smarter]

I suggest making a PR to scala/scala that drops the feature and put a warning in its place.

[eparejatobes]

this is a terrible feature that should be removed :).

Would love to see both reasons and tests for this. I see https://github.com/sbt/zinc/issues/537#issuecomment-421637743 as a much more reasonable compromise.

[som-snytt]

Currently (2.13.12),

$ scalac '-opt:inline:<sources>' -d /tmp/sandbox src/main/scala/separateFile.scala src/main/scala/4125.scala
$ javap -c '/tmp/sandbox/issues/sbt4125/useInlinedStuff$.class'
Compiled from "separateFile.scala"
public final class issues.sbt4125.useInlinedStuff$ {
  public static final issues.sbt4125.useInlinedStuff$ MODULE$;

  public static {};
    Code:
       0: new           #2                  // class issues/sbt4125/useInlinedStuff$
       3: dup
       4: invokespecial #12                 // Method "<init>":()V
       7: putstatic     #14                 // Field MODULE$:Lissues/sbt4125/useInlinedStuff$;
      10: return

  public int z();
    Code:
       0: iconst_1
       1: getstatic     #21                 // Field issues/sbt4125/inlineFunTimes$.MODULE$:Lissues/sbt4125/inlineFunTimes$;
       4: pop
       5: iconst_1
       6: iadd
       7: ireturn
}

The inline annotation is not required.

Also, everyone is 5 years older since the previous comments.

[Friendseeker]

The inline annotation is not required.

Yeah... I honestly thought that https://github.com/sbt/zinc/pull/1310 would cut it, until I tested the PR fix against 2.13.12...

Also, everyone is 5 years older since the previous comments.

Maybe I can advertise myself as a historian as I now have experience with digging up 5 year old archaic zinc ticket.