inlining changes needed

[martin odersky]

So, I made the changes to the inliner that all fields and methods accessed by an @inline method are made not private. But the inliner still did not work for them, when compiled separately. I could trace the reason to the (now obsolete) "potentially publized" logic, which will refuse to inline any separately compiled reference that has a $ in it. I submitted a pull request that disregards "potentiallyPublicized" when checking fields for inlineability. 

I verified that the change reduces the number of "cannot inline" warnings for Typers.scala from ~250 to 19.

But I could not do a complete reversal of the publication logic to make the change safe. Vlad or Miguel, can you have a look at this? In particular, I think the inliner should no longer make private fields public. 

Thanks!

 - Martin

--
Martin Odersky
Prof., EPFL and Chairman, Typesafe
PSED, 1015 Lausanne, Switzerland
Tel. EPFL: +41 21 693 6863
Tel. Typesafe: +41 21 691 4967

[Grzegorz Kossakowski]

I found the same and had a pull request cooked but I had no internet access to push.

On library, reflect and compiler the number of warning is reduced by 50%. Lots of remaining ones are related to Predef.->. I'll follow up with a ticket.

--
Sent from my Android
Grzegorz

[Miguel Garcia]

My status report, for the record.

The approach I'm following to inline trait methods [1] seems "almost ready" (optimized bootstraps and tests pass) except that I was getting a VerifyError in the nightly.

This one was hard to find, and results from ICodeReader delivering the following ICode [2]

  LOAD_FIELD scala.reflect.internal.ClassfileConstants$FlagTranslation
  LOAD_LOCAL(variable par1)
  CALL_METHOD scala.reflect.internal.ClassfileConstants$FlagTranslation.methodFlags (dynamic)
  RETURN(LONG)

when in fact it should provide instead:

  LOAD_MODULE object ClassfileConstants$FlagTranslation <----------- this is the line that matters
  LOAD_LOCAL(value flags)
  CALL_METHOD scala.reflect.internal.ClassfileConstants$FlagTranslation.methodFlags (dynamic)
  RETURN(LONG)

The former makes the backend emit the all-crappy:

   13:    getstatic    #817; //Field scala/reflect/internal.scala/reflect/internal/ClassfileConstants$FlagTranslation:Lscala/reflect/internal/ClassfileConstants$FlagTranslation;
   16:    iload    19
   18:    invokevirtual    #821; //Method scala/reflect/internal/ClassfileConstants$FlagTranslation.methodFlags:(I)J

I'm mentioning this because Vlad is also finding some interesting ICodeReader behaviors.

You see, getting Inliner and lookupIMethod() right was the easy part. Fixing ClassfileParser and ICodeReader come next.


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




[1] https://github.com/magarciaEPFL/scala/compare/scala:2.10.x...SI-4767-b

[2] when reading scala.reflect.internal.ClassfileConstants.toScalaMethodFlags()

[√iktor Ҡlang]

I stumbled unto a bug with package-protected Java access & -optimize (with @inline) if/when I have time I'll try to minimize it.

Cheers,

√

--
Viktor Klang

Akka Tech Lead

Typesafe - The software stack for applications that scale

Twitter: @viktorklang

[Adriaan Moors]

Excellent! Glad to see such good progress.

For the record, I'm not surprised ClassFileParser/ICodeReader are the culprit and not the inliner.

I think we all came to that conclusion independently when initially diagnosing the bug.

I'm sorry if that wasn't clearer from earlier discussions.

adriaan

PS: I'm sure mister $dollar sign is implicated somehow as well.

On Thu, Aug 9, 2012 at 11:47 PM, Miguel Garcia <miguel...@tuhh.de> wrote:

[Miguel Garcia]

I patched the Inliner to prevent it from making any fields public, and noticed several anon-closures not being inlined anymore as hinted by:

  instruction LOAD_FIELD value $outer requires private access.

The current version of  canMakePublic() makes public outer fields, as well as others. It can be made more restrictive.
The alternative, regarding all apply methods of anon-closures as @inline annotated, would have to deal with outer fields being introduced after SuperAccessors has run.

    def canMakePublic(f: Symbol): Boolean =
      (m.sourceFile ne NoSourceFile) &&
      (f.isSynthetic || f.isParamAccessor) &&
      { toBecomePublic = f :: toBecomePublic; true }


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

On Thursday, August 9, 2012 8:11:18 PM UTC+2, martin wrote:

[martin odersky]

On Fri, Aug 10, 2012 at 12:00 PM, Miguel Garcia <miguel...@tuhh.de> wrote:

I patched the Inliner to prevent it from making any fields public, and noticed several anon-closures not being inlined anymore as hinted by:

  instruction LOAD_FIELD value $outer requires private access.

The current version of  canMakePublic() makes public outer fields, as well as others. It can be made more restrictive.
The alternative, regarding all apply methods of anon-closures as @inline annotated, would have to deal with outer fields being introduced after SuperAccessors has run.

    def canMakePublic(f: Symbol): Boolean =
      (m.sourceFile ne NoSourceFile) &&
      (f.isSynthetic || f.isParamAccessor) &&
      { toBecomePublic = f :: toBecomePublic; true }

I see. So what do we do? For the moment: Publicize outer fields? But that would not work for separate compilation, right?

Do we have an example how much is effected by this?

I think we can fix it so that outer fields of classes containing @inlined methods are always made public, or we have a more precise analysis. But that's maybe post 2.10.

 - Martin

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

On Thursday, August 9, 2012 8:11:18 PM UTC+2, martin wrote:

So, I made the changes to the inliner that all fields and methods accessed by an @inline method are made not private. But the inliner still did not work for them, when compiled separately. I could trace the reason to the (now obsolete) "potentially publized" logic, which will refuse to inline any separately compiled reference that has a $ in it. I submitted a pull request that disregards "potentiallyPublicized" when checking fields for inlineability. 

I verified that the change reduces the number of "cannot inline" warnings for Typers.scala from ~250 to 19.

But I could not do a complete reversal of the publication logic to make the change safe. Vlad or Miguel, can you have a look at this? In particular, I think the inliner should no longer make private fields public. 

Thanks!

 - Martin

--
Martin Odersky
Prof., EPFL and Chairman, Typesafe
PSED, 1015 Lausanne, Switzerland
Tel. EPFL: +41 21 693 6863
Tel. Typesafe: +41 21 691 4967

[Miguel Garcia]

> So what do we do? For the moment: Publicize outer fields? But that would not work for separate compilation, right?

Marking as public the outer fields of anon-closures (and only those) shouldn't pose a problem with separate compilation.

Pretty much all non-trivial anon-closures-classes couldn't be eliminated otherwise (once inlined, the code still has to reach via the outer field to environ values).

In fact Inliner already performs a test for that (in other places):

  /** Is the given class a closure? */
  def isClosureClass(cls: Symbol): Boolean =
    cls.isFinal && cls.isSynthetic && !cls.isModuleClass && cls.isAnonymousFunction


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

[Martin Odersky]

I think that's a valid option. We can refine it later. I am away now so can't do anythin until next thursday. But hopefully we'll have a fix sooner than that.

Cheers

  Martin 

Sent from my phone

[martin odersky]

Here's the conundrum for access inlining: We can make non-private everything an @inline method accesses. Ideally this is done in ExplicitOuter, where other access widenings also take place. However, the widenings prompted by separately compiled @inline methods will not show in pickled information. So the inliner will still think fields accessed by these methods are private and refuse to to its work. 

To work around this, I have moved access widenings to SuperAccessors, which runs before pickling. But that does not widen outer fields which are only introduced in explicit outer. So we still miss on a complete solution. 

We could of course indiscriminately make all outer fields public, but this is a rather crude hammer. 

It strikes me there is a more elegant solution: First, let's widen everything accessed by @inline methods and let's do it in ExplicitOuter. Then, let's modify the inliner that it retro-actively widens everything accessed by a method that it is going to inline, and let's do this whether separately compiled or not. 

So, to get there: Move things back to ExplicitOuter, and change the inliner so that it always makes fields accessed by @inline methods public. 

Miguel's pending patch for anonymous classes would no longer be needed.

Cheers

 - Martin

[Miguel Garcia]

(In order to synchronize with Greg, some implementation details)

Following that approach, the change in the inliner amounts to:

    def canMakePublic(f: Symbol): Boolean =

      (m.sourceFile ne NoSourceFile) && (m.sourceFile ne null) && // "compiled in this run" ie not grabbed from external library
      hasInline(f.owner) &&

      { toBecomePublic = f :: toBecomePublic; true }

ie only those private fields of a class being compiled that are accessed in @inline-marked methods are publicized.

Just to recap, ExplicitOuter should have taken care of publicizing all other fields required for inlining (e.g., outer fields of anon-closure-classes)


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

[martin odersky]

On Sat, Aug 11, 2012 at 7:30 PM, Miguel Garcia <miguel...@tuhh.de> wrote:

(In order to synchronize with Greg, some implementation details)

Following that approach, the change in the inliner amounts to:

    def canMakePublic(f: Symbol): Boolean =

      (m.sourceFile ne NoSourceFile) && (m.sourceFile ne null) && // "compiled in this run" ie not grabbed from external library
      hasInline(f.owner) &&

      { toBecomePublic = f :: toBecomePublic; true }

ie only those private fields of a class being compiled that are accessed in @inline-marked methods are publicized.

Just to recap, ExplicitOuter should have taken care of publicizing all other fields required for inlining (e.g., outer fields of anon-closure-classes)

Exactly. 

The explicitouter change is here:

https://github.com/scala/scala/pull/1121 

Cheers

[Grzegorz Kossakowski]

On 11 August 2012 19:30, Miguel Garcia <miguel...@tuhh.de> wrote:

(In order to synchronize with Greg, some implementation details)

Following that approach, the change in the inliner amounts to:

    def canMakePublic(f: Symbol): Boolean =

      (m.sourceFile ne NoSourceFile) && (m.sourceFile ne null) && // "compiled in this run" ie not grabbed from external library
      hasInline(f.owner) &&

      { toBecomePublic = f :: toBecomePublic; true }

ie only those private fields of a class being compiled that are accessed in @inline-marked methods are publicized.

Just to recap, ExplicitOuter should have taken care of publicizing all other fields required for inlining (e.g., outer fields of anon-closure-classes)

I think I got lost here. Why ExplicitOuter cannot make public everything that is needed so Inliner doesn't change access levels at all?

--
Grzegorz Kossakowski

[martin odersky]

Because of separate compilation: The inliner looks at the Scala types, not at the Java types. That's potentially a mistake, it might have been more robust to look at the Java types. But that's how inliner works now. 

Now, looking at the Scala types, the inliner sees what got pickled subject to any symbol info transforms afterwards. Crucially, the symbol info transforms will never look at internal code of separately compiled classes. So if ExplicitOuter for a separately compiled class decides that a field should be public, that info cannot be propagated to separately compiled clients.

But as long as both classes agree on the "meta-knowledge" that all fields accessed by @inline will be made public they can still work OK together. It's just that clients will have to manually duplicate the access widening transform that they know the server will have performed.

Cheers

 - Martin

--
Grzegorz Kossakowski

[Grzegorz Kossakowski]

On 13 August 2012 12:41, martin odersky <martin....@epfl.ch> wrote:

Because of separate compilation: The inliner looks at the Scala types, not at the Java types. That's potentially a mistake, it might have been more robust to look at the Java types. But that's how inliner works now. 

Now, looking at the Scala types, the inliner sees what got pickled subject to any symbol info transforms afterwards. Crucially, the symbol info transforms will never look at internal code of separately compiled classes. So if ExplicitOuter for a separately compiled class decides that a field should be public, that info cannot be propagated to separately compiled clients.

But as long as both classes agree on the "meta-knowledge" that all fields accessed by @inline will be made public they can still work OK together. It's just that clients will have to manually duplicate the access widening transform that they know the server will have performed.

Ok, I got it now. Thanks for explanation!

It sounds good to me as well.

--
Grzegorz Kossakowski

[Jason Zaugg]

Friends of @inline,

I've been taking a look at this issue in the context of fixing the
regression SI-6562.

Could you please take a look at my comments on that ticket and at this branch?

https://github.com/retronym/scala/compare/2.10.0-wip...ticket/6562-3

In particular, I'm after a test case for the problem with closure
inlining that Miguel observed earlier in this thread.

Rolling out incremental improvements in 2.10.1 will be tricky: the
inliner must be able to handle the access widening schemes of both
2.10.0 and 2.10.1. But at that stage we could bite the bullet and base
the PRIVATE flag on the Java signatures, rather than the
(pre-ExplicitOuter) pickled signatures. Then the inliner need not have
the tacit knowledge of what has gone before it.

-jason