Inliner gives up too early?

399 views
Skip to first unread message

Grzegorz Kossakowski

unread,
Aug 11, 2012, 8:41:53 AM8/11/12
to scala-internals, Garcia Gutierrez Miguel Alfredo
Hi,

I'm trying to compile the following example:

class Foo {
  def foo: Map[String, String] = Map[String, String](
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar",
      "foo" -> "bar"
    )
}

with -optimize -Yinline-warnings and I get the following output:

warning: At the end of the day, could not inline @inline-marked method minus>

/Users/grek/scala/scala/Arrow.scala:23: warning: At the end of the day, could not inline @inline-marked method any2ArrowAssoc

      "foo" -> "bar",

      ^

/Users/grek/scala/scala/Arrow.scala:24: warning: At the end of the day, could not inline @inline-marked method any2ArrowAssoc

      "foo" -> "bar"

      ^

warning: At the end of the day, could not inline @inline-marked method minus>

warning: At the end of the day, could not inline @inline-marked method minus>

5 warnings found


I debugged the inliner and it turns out that it simply gives up too early. The relevant line is Inliners.scala:561:

      while (retry && count < MAX_INLINE_RETRY)

Here the count is greater than MAX_INLINE_RETRY so inliner just gives up. I'm wondering if it would make sense to tweak the condition so inliner always tries to inline everything that is marked with @inline and only if it exhausts all options (so it inlines everything or there's no way to continue inlining) it abort. Basically, the idea is that @inline annotation enforces inlining no matter what heuristics say.

Miguel, would that make sense in your opinion?

-- 
Grzegorz




--
Grzegorz Kossakowski

Miguel Garcia

unread,
Aug 11, 2012, 9:07:13 AM8/11/12
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
Greg,

In that case, upon inlining, you want `count` not to be increased when the callee is marked @inline. The relevant line is:

  if (isCountable) { count += 1 };

Currently `isCountable()` doesn't do what you want, but can be easily modified, as follows:

   /* count only callees other than:
    *  (a) methods owned by anon-closure-classes;
    *  (b) @inline-marked methods;
    *  (c) `foreach`, `filter`, `withFilter`, `map`, `flatMap`; and
    *  (d) those in RuntimePackage.
    */
   val isCountable = !(
        isClosureClass(receiver)
     || hasInline(concreteMethod) // <------- just added
     || hasInline(inc.sym)        // <------- just added
     || isMonadicMethod(concreteMethod)
     || receiver.enclosingPackage == definitions.RuntimePackage
   )

   if (isCountable) { count += 1 };


The updated definition of `isCountable` now appears right before the single use it sees. Readability.


Miguel
http://lampwww.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/

Grzegorz Kossakowski

unread,
Aug 11, 2012, 10:10:29 AM8/11/12
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
On 11 August 2012 15:07, Miguel Garcia <miguel...@tuhh.de> wrote:
Greg,

In that case, upon inlining, you want `count` not to be increased when the callee is marked @inline. The relevant line is:

  if (isCountable) { count += 1 };

Currently `isCountable()` doesn't do what you want, but can be easily modified, as follows:

   /* count only callees other than:
    *  (a) methods owned by anon-closure-classes;
    *  (b) @inline-marked methods;
    *  (c) `foreach`, `filter`, `withFilter`, `map`, `flatMap`; and
    *  (d) those in RuntimePackage.
    */
   val isCountable = !(
        isClosureClass(receiver)
     || hasInline(concreteMethod) // <------- just added
     || hasInline(inc.sym)        // <------- just added
     || isMonadicMethod(concreteMethod)
     || receiver.enclosingPackage == definitions.RuntimePackage
   )

   if (isCountable) { count += 1 };


The updated definition of `isCountable` now appears right before the single use it sees. Readability.

Oh cool! Thanks for the pointer in the right direction. I'll check your patch right away.

--
Grzegorz Kossakowski

Grzegorz Kossakowski

unread,
Aug 11, 2012, 10:29:43 AM8/11/12
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
On 11 August 2012 15:07, Miguel Garcia <miguel...@tuhh.de> wrote:
Greg,

In that case, upon inlining, you want `count` not to be increased when the callee is marked @inline. The relevant line is:

  if (isCountable) { count += 1 };

Currently `isCountable()` doesn't do what you want, but can be easily modified, as follows:

   /* count only callees other than:
    *  (a) methods owned by anon-closure-classes;
    *  (b) @inline-marked methods;
    *  (c) `foreach`, `filter`, `withFilter`, `map`, `flatMap`; and
    *  (d) those in RuntimePackage.
    */
   val isCountable = !(
        isClosureClass(receiver)
     || hasInline(concreteMethod) // <------- just added
     || hasInline(inc.sym)        // <------- just added

What's inc here? I cannot apply this patch on top of current 2.10.x branch.

--
Grzegorz Kossakowski

Miguel Garcia

unread,
Aug 11, 2012, 11:56:52 AM8/11/12
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
The following commit contains that patch and compiles:
  https://github.com/magarciaEPFL/scala/commit/39007f1d51affb9b56b4d4347ada26574b49960b

`inc` is a wrapper (IMethodInfo) that provides some utilities to find things about the callee.

Talking about patches to the inline, here's the PR from yesterday about publicizing outer fields only:
  https://github.com/scala/scala/pull/1115


Miguel
http://lampwww.epfl.ch/~magarcia/ScalaCompilerCornerReloaded/

Grzegorz Kossakowski

unread,
Aug 12, 2012, 6:35:47 AM8/12/12
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
On 11 August 2012 17:56, Miguel Garcia <miguel...@tuhh.de> wrote:
The following commit contains that patch and compiles:
  https://github.com/magarciaEPFL/scala/commit/39007f1d51affb9b56b4d4347ada26574b49960b

`inc` is a wrapper (IMethodInfo) that provides some utilities to find things about the callee.

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.
 
Talking about patches to the inline, here's the PR from yesterday about publicizing outer fields only:
  https://github.com/scala/scala/pull/1115

I'll have a look into Martin's discussion.

--
Grzegorz Kossakowski

Paolo G. Giarrusso

unread,
Mar 12, 2014, 3:50:04 PM3/12/14
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
On Sunday, August 12, 2012 12:35:47 PM UTC+2, Grzegorz Kossakowski wrote:
On 11 August 2012 17:56, Miguel Garcia <miguel...@tuhh.de> wrote:
The following commit contains that patch and compiles:
  https://github.com/magarciaEPFL/scala/commit/39007f1d51affb9b56b4d4347ada26574b49960b

`inc` is a wrapper (IMethodInfo) that provides some utilities to find things about the callee.

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.
(In fact, I'm interested mostly because of the compiler warnings, not so much because of the performance).

Grzegorz Kossakowski

unread,
Mar 12, 2014, 4:47:08 PM3/12/14
to scala-internals, Garcia Gutierrez Miguel Alfredo
The status quo has been kept in 2.11.

If we were talking about 2-3% performance drop then I would agree that making optimizer work harder should be the way to go. However, almost 20% drop is not something we can seriously consider. However, my test was based on just increasing number of iterations and not on changing the definition of isCountable as Miguel suggested in this thread. We would have to try it and see what's the performance difference.

We can consider this for Scala 2.11.1.

--
Grzegorz Kossakowski
Scalac hacker at Typesafe
twitter: @gkossakowski

iulian dragos

unread,
Mar 13, 2014, 11:22:12 AM3/13/14
to scala-i...@googlegroups.com, Garcia Gutierrez Miguel Alfredo
Greg, the whole point of 'isCountable' is precisely to keep inlining under a reasonable budget. Definitely increasing the retry count is the wrong way about it. I bet that the number of @inline methods is much smaller than the total count of methods that could be inlined.

 

We can consider this for Scala 2.11.1.

--
Grzegorz Kossakowski
Scalac hacker at Typesafe
twitter: @gkossakowski

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.



--
« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

Grzegorz Kossakowski

unread,
Mar 13, 2014, 12:11:06 PM3/13/14
to scala-internals
On 13 March 2014 16:22, iulian dragos <jagu...@gmail.com> wrote:

Greg, the whole point of 'isCountable' is precisely to keep inlining under a reasonable budget. Definitely increasing the retry count is the wrong way about it. I bet that the number of @inline methods is much smaller than the total count of methods that could be inlined.

Iulian, I don't understand that part of inliner so it's great you joined the conversation :-)

Does each iteration inline just one method invocation?

What would be a good way to convince inliner to always inline all method calls marked with @inline?

iulian dragos

unread,
Mar 14, 2014, 9:29:03 AM3/14/14
to scala-i...@googlegroups.com
On Thu, Mar 13, 2014 at 5:11 PM, Grzegorz Kossakowski <grzegorz.k...@gmail.com> wrote:
On 13 March 2014 16:22, iulian dragos <jagu...@gmail.com> wrote:

Greg, the whole point of 'isCountable' is precisely to keep inlining under a reasonable budget. Definitely increasing the retry count is the wrong way about it. I bet that the number of @inline methods is much smaller than the total count of methods that could be inlined.

Iulian, I don't understand that part of inliner so it's great you joined the conversation :-)

Does each iteration inline just one method invocation?

What would be a good way to convince inliner to always inline all method calls marked with @inline?

I believe Miguel's suggestion is the right one: don't count methods annotated with @inline against the "budget" of inlining. The `isCountable` method exists in both 2.10 and 2.11, so it shouldn't be too hard to backport a fix.

iulian
 


--
Grzegorz Kossakowski
Scalac hacker at Typesafe
twitter: @gkossakowski
github: @gkossakowski

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply all
Reply to author
Forward
0 new messages