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

2.10 regression: inliner warnings with Map literal and -optimize #6723

Closed
scabug opened this Issue Nov 26, 2012 · 19 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Nov 26, 2012

here's the code:

% cat S.scala                                  
object O { Map(
  "a" -> 0, "b" -> 1, "c" -> 2, "d" -> 3, "e" -> 4,
  "a" -> 0, "b" -> 1, "c" -> 2, "d" -> 3, "e" -> 4,
  "a" -> 0, "b" -> 1, "c" -> 2, "d" -> 3, "e" -> 4,
  "f" -> 5, "g" -> 6, "h" -> 7, "i" -> 8) }

2.9 with -optimize likes it fine:

% scalac29 -version
Scala compiler version 2.9.2 -- Copyright 2002-2011, LAMP/EPFL
% scalac29 -optimize S.scala

but with 2.10 (RC2) the inliner starts whining:

% scalac210 -version
Scala compiler version 2.10.0-RC2 -- Copyright 2002-2012, LAMP/EPFL
% scalac210 -optimize S.scala
warning: there were 3 inliner warnings; re-run with -Yinline-warnings for details
one warning found
% scalac210 -optimize -Yinline-warnings S.scala
warning: At the end of the day, could not inline @inline-marked method ->$extension
warning: At the end of the day, could not inline @inline-marked method ->$extension
S.scala:3: warning: At the end of the day, could not inline @inline-marked method any2ArrowAssoc
  "f" -> 5, "g" -> 6, "h" -> 7, "i" -> 8) }                                ^
three warnings found

The map has to be a certain length before the warnings start appearing.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 26, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6723?orig=1
Reporter: @SethTisue
Affected Versions: 2.10.0-RC2

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 26, 2012

@SethTisue said:
one possible response is, "what, you really expect -optimize not to cause tons of warnings?"

but actually, for whatever it's worth, RC2 with -optimize compiles my entire large codebase (100K+ lines of mixed Scala and Java) with no other warnings produced besides this one (246 times). tests pass, too. so I'm thiiiiiis close to being able to use -optimize and -Xfatal-warnings. which would be pretty cool and help the optimizer get tested.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 26, 2012

@magarciaEPFL said:

The map has to be a certain length before the warnings start appearing.

That, and the -Xprint:cleanup snippet below, hint at the cause of the warnings: the inlining heuristics back off after method size surpasses a threshold.

scala.Predef.Map().apply(
  scala.Predef.wrapRefArray(
    Array[Tuple2]{
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("a"), scala.Int.box(0)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("b"), scala.Int.box(1)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("c"), scala.Int.box(2)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("d"), scala.Int.box(3)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("e"), scala.Int.box(4)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("f"), scala.Int.box(5)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("g"), scala.Int.box(6)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("h"), scala.Int.box(7)),
      Predef$ArrowAssoc.->$extension(scala.Predef.any2ArrowAssoc("i"), scala.Int.box(8))
    }.$asInstanceOf[Array[Object]]()
  )
);
@scabug

This comment has been minimized.

Copy link
Author

scabug commented Dec 17, 2012

@non said:
I hit this issue also.

In my case I solved it by using (x, y) instead of x -> y. Since most users probably think of -> as syntax (and not as an implicitly called extension method on a value class) it would be nice if the inlining could be forced in cases like this.

@scabug

This comment has been minimized.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jun 25, 2013

Martin Kneissl (mkneissl) said:
Problem still present in 2.10.2 .

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 10, 2014

Sarah Gerweck (gerweck) said:
If nobody has the time or interest to fix this for 2.11, maybe the @inline could be removed from that method? Or a special case to disable the warning for arrows?

It's a little annoying to get hundreds of warnings because of this. The cost of disabling the Scala optimizer is too high to disable it just because of this, but it's hard to find your real warnings in the midst of all the noise this generates.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Mar 12, 2014

@Blaisorblade said:
Grzegorz Kossakowski wrote:

I see. I didn't know you moved the method. I tried your commit and it helps with getting rid of 586 of inliner warnings. However, the price seems to be rather high: ant build-opt takes 19% longer on machine.

What happened out of this? Getting worse performance for more optimization, by itself, should not stop such a patch being merged.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Mar 12, 2014

@gkossakowski said:
Consider tweaking definition of isCountable in Scala 2.11.1: https://groups.google.com/d/msg/scala-internals/5wxUElooFl4/p2A1678p1XQJ

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Aug 5, 2014

@gkossakowski said:
The 2.11.2 is out so I'm rescheduling the issue for 2.11.3.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 9, 2014

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 4, 2014

@retronym said:
Updating fix-by version to 2.11.5.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 8, 2015

Malcolm Greaves (malcolmgreaves) said:
It's 2.11.6 and this is still unresolved...any updates?

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 8, 2015

@adriaanm said:
We're very actively working on a new optimizer (based on Miguel's work). By "we" I mean Lukas (the assignee). Check it out in 2.11.6 with -Ybackend:GenBCode -- will be default in 2.12.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented May 10, 2015

Malcolm Greaves (malcolmgreaves) said:
Oh fantastic! I'll give this a whirl on my code where I encounter this bug.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 5, 2016

Malcolm Greaves (malcolmgreaves) said:
Wanted to chime in -- I did find that using this new backend solved this problem for me. Thank you!

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Apr 4, 2016

@szeiger said:
@lrytz, is there anything left to do here or can this be closed?

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Apr 4, 2016

@lrytz said:
it's kind of related to implementing the final version of the inliner heuristics; currently we just inline without even considering the final method size, which might change, and therefore impact this issue

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Apr 6, 2016

@SethTisue said (edited on Apr 6, 2016 7:33:17 PM UTC):
I am closing this. The original issue I reported is fixed. It's now perfectly safe to use the optimizer with, at least, medium-sized Map literals, the sort one might normally use in normal code.

I was able to get the new optimizer to produce the following warning, but only by cranking the size of the map up really high (thousands):

[warn] scala/Predef$ArrowAssoc$::$minus$greater$extension(Ljava/lang/Object;Ljava/lang/Object;)Lscala/Tuple2; is annotated @inline but could not be inlined:
[warn] The size of the callsite method O$::<init>()V
[warn] would exceed the JVM method size limit after inlining scala/Predef$ArrowAssoc$::$minus$greater$extension(Ljava/lang/Object;Ljava/lang/Object;)Lscala/Tuple2;.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.