stability failure due to interaction of -optimize with pullreq 402

[Adriaan Moors]

pull request 402 [*] fails test.stability under -optimise (non-optimised, it's fine)

the diff of javap output:

http://dl.dropbox.com/u/12862572/stability.diff (hat-tip to libscala's textconv for .class files)

basically, some private fields that correspond to primary ctor args have been made public the second time around,

nothing else has changed

test.stability using -optimise -Xoldpatmat is also fine, and all #402 changes is how PartialFunctions are synthesized,

so the cause must be in typedMatchAnonFun (but I'm tempted to say the underlying bug is in the inliner)

this is very hard to reproduce, because simply taking the strap compiler and using it to compile, say MatchError

(one of the classes whose bytecode was changed), yields the same results as using the quick compiler

I could really use some help here -- I'm stumped.

Also, I'd really, really like to get #402 in, because it fixes some major, major bugs in how virtpatmat deals with PartialFunction

[*] https://github.com/scala/scala/pull/402

[Paul Phillips]

On Sun, Apr 15, 2012 at 3:00 PM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> (but I'm tempted to say the underlying bug is in the inliner)

Do not wish to derail your question, but we should answer that question.

https://issues.scala-lang.org/browse/SI-5442

I would think one should have to "opt in" to creating bytecode which
will not interoperate with other bytecode, more so than giving
"-optimise" represents opting in. I want things the optimizer does,
some of which are relatively simple but critical to the quality of the
bytecode, without taking a hit like that.

Basically, throughout the compiler things have their access altered
fairly casually as if it were a decision without impact beyond the
disappointment one might feel at seeing declared private things
public; when in fact it is a huge far-reaching decision to change the
access of something, and the slightest misstep will gift you with
runtime failures and/or nondeterministic compilation products.

[Adriaan Moors]

Agreed. So, I'm blocked until this bug is fixed.

I'm afraid I'll have to revert my commit that enables the new pattern matcher by default unless this is resolved.

I don't want to release something that I know to be broken, and know how to fix. (But can't.)

[Vlad Ureche]

On Sun, Apr 15, 2012 at 8:35 PM, Adriaan Moors <adriaa...@epfl.ch> wrote:

Agreed. So, I'm blocked until this bug is fixed.

I'm afraid I'll have to revert my commit that enables the new pattern matcher by default unless this is resolved.

I don't want to release something that I know to be broken, and know how to fix. (But can't.)

Will look into it now. If you don't have a patch by tomorrow morning at least you'll have some conclusions that Miguel can use to further debug it.

On Sun, Apr 15, 2012 at 4:18 PM, Paul Phillips <pa...@improving.org> wrote:

On Sun, Apr 15, 2012 at 3:00 PM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> (but I'm tempted to say the underlying bug is in the inliner)

Do not wish to derail your question, but we should answer that question.

 https://issues.scala-lang.org/browse/SI-5442

I would think one should have to "opt in" to creating bytecode which
will not interoperate with other bytecode, more so than giving
"-optimise" represents opting in.  I want things the optimizer does,
some of which are relatively simple but critical to the quality of the
bytecode, without taking a hit like that.

Basically, throughout the compiler things have their access altered
fairly casually as if it were a decision without impact beyond the
disappointment one might feel at seeing declared private things
public; when in fact it is a huge far-reaching decision to change the
access of something, and the slightest misstep will gift you with
runtime failures and/or nondeterministic compilation products.

This bug is not related to virtPartMat's code generation. And thinking more about it led to the conclusion that @inline and -optimize should be treated separately:
 - when encountering @inline functions, scalac should either transform all members accessed by the @inline function to public and issue member public-isation warnings OR if it can't make the members public (e.g. one lies in a class that is not being recompiled) just remove the @inline flag and once again warn.
 - actual inlining should be done after this analysis and public-isation phase, only if -optimize is added
The motivation is: if you care about your members remaining private/protected, don't use @inline. If you're okay with the fields becoming public, ignore the warning and go on.

Unfortunately this won't make it into M3, as it takes quite a bit of work to come up with a decent implementation and testcases for everything.

Cheers,

Vlad Ureche

[Vlad Ureche]

On Sun, Apr 15, 2012 at 10:00 PM, Vlad Ureche <vlad....@gmail.com> wrote:

Basically, throughout the compiler things have their access altered
fairly casually as if it were a decision without impact beyond the
disappointment one might feel at seeing declared private things
public; when in fact it is a huge far-reaching decision to change the
access of something, and the slightest misstep will gift you with
runtime failures and/or nondeterministic compilation products.

This bug is not related to virtPartMat's code generation. And thinking more about it led to the conclusion that @inline and -optimize should be treated separately:
 - when encountering @inline functions, scalac should either transform all members accessed by the @inline function to public and issue member public-isation warnings OR if it can't make the members public (e.g. one lies in a class that is not being recompiled) just remove the @inline flag and once again warn.
 - actual inlining should be done after this analysis and public-isation phase, only if -optimize is added
The motivation is: if you care about your members remaining private/protected, don't use @inline. If you're okay with the fields becoming public, ignore the warning and go on.

Unfortunately this won't make it into M3, as it takes quite a bit of work to come up with a decent implementation and testcases for everything.

I had another shot at this. Forget what I wrote above, it's wrong, it still can't get through recompilation correctly.

The new solution I propose involves adding a contract that makes class C expose its private members. I propose that we use a marker trait, InlineReady that will make all of C's synthetic members public. There are several advantages to it:
 - we separate the process of making members public and the process of inlining
 - there's an explicit contract binding C to make its synthetic members public (and that is AGREED by the programmer explicitly)
 - the inheritance propagates transitively, thus if D <:< C, then it follows that D is also InlineReady
 - the inliner is not allowed to change access rights anymore. If a programmer-declared member is private or protected and can't be accessed, the programmer gets a warning.

What do you think?

PS: I implemented part 3 of the idea (no more changing access rights) and put it in pull request 408. It makes the inliner almost useless in its current format, but passes the stability checks. Please let me know if I should go ahead with the InlineReady approach.

Thanks,
Vlad

[Adriaan Moors]

How about generating a name-mangled public forwarder to the private method that is needed elsewhere for inlining to succeed?

[Vlad Ureche]

On Mon, Apr 16, 2012 at 11:30 AM, Adriaan Moors <adriaa...@epfl.ch> wrote:

How about generating a name-mangled public forwarder to the private method that is needed elsewhere for inlining to succeed?

Yep, that's a great idea. It would prevent accidental private field access by the programmer.

Cheers,
Vlad

[Miguel Garcia]

Hi,

I've made some progress regarding the root cause of the test.stability failure reported in [1]

To find what the inliner does differently, I started by reverting the last band-aid [2] to later compare the -Ylog:inliner output across compiler runs.

That output can be obtained by:

 (1) "ant all.clean build"
 (2) use the resulting compiler and library from build\pack\lib to compile the compiler and library sources.
 (3) run the compiler and library emitted above, on the same sources.

In both cases with "-optimize -Ylog:inliner"

The logs reveal some inlinings occurring in the first but not in the second compilation (I tried also a third compilation, but the logs were identical to those for the second). Details about those inlinings below. They're about Range.{apply, inclusive} and RichInt.to.

"Some progress" because there are at least four pending questions:

  (a) why that happens (macros at play for Range, RichInt?)

  (b) a more fine-grain comparison (per classfile) via -Ygen-javap

  (c) going over the logs for the other optimization phases

  (d) adopting the policy to 'publicize' less aggressively, allowing it only when both callee and caller are defined in the same compilation unit, ie
        def makePublic(f: Symbol): Boolean =
          (inc.m.sourceFile ne NoSourceFile)        &&
          (inc.m.sourceFile == caller.m.sourceFile) &&
          (f.isSynthetic || f.isParamAccessor)      && {
            debuglog("Making not-private symbol out of synthetic: " + f)
            f setNotFlag Flags.PRIVATE
            true
          }

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


[1] https://groups.google.com/d/topic/scala-internals/yGmOkBn9Gmk/discussion

[2] https://github.com/scala/scala/commit/23ae258c37bd5de050ae9d8b4287dc8c309af8de


[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.ast.TreeBrowsers$BrowserFrame._setExpansionState$1 at pos: 4579
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.backend.jvm.GenJVM$BytecodeGenerator.emitArgument$1 at pos: 26801
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.backend.msil.GenMSIL$BytecodeGenerator.addSymtabAttribute at pos: 9093
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.backend.msil.GenMSIL$BytecodeGenerator.createTypeBuilder at pos: 75025
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.backend.msil.GenMSIL$BytecodeGenerator.scala$tools$nsc$backend$msil$GenMSIL$BytecodeGenerator$$createDelegateCaller at pos: 86815
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.backend.msil.GenMSIL$BytecodeGenerator.scala$tools$nsc$backend$msil$GenMSIL$BytecodeGenerator$$createDelegateCaller at pos: 87396
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.interactive.tests.Tester$Change.deleteAll at pos: 2709
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.interactive.tests.Tester.run at pos: 5428
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.interactive.tests.Tester.testFileChanges at pos: 4000
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.scratchpad.CommentWriter.write at pos: 702
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.scratchpad.SourceInserter.skip at pos: 2628
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.scratchpad.SourceInserter.write at pos: 1994
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseAnnotArg$1 at pos: 37699
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseAnnotation$1 at pos: 38984
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseAnnotations$1 at pos: 41113
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseAttributes at pos: 41655
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseExceptions$1 at pos: 40603
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseInnerClasses at pos: 43605
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseInnerClasses at pos: 43610
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.parseScalaLongSigBytes$1 at pos: 38413
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.skipAttributes at pos: 48298
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ClassfileParser.skipMembers at pos: 48438
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ICodeReader.parseClass at pos: 2178
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ICodeReader.parseClass at pos: 2259
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.symtab.classfile.ICodeReader.parseMethod at pos: 4685
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.typechecker.RefChecks$RefCheckTransformer.validateBaseTypes at pos: 38061
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.typechecker.RefChecks$RefCheckTransformer.validateBaseTypes at pos: 38862
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.util.BatchSourceFile.calculateLineIndices at pos: 4905
[log inliner] Inlining scala.collection.immutable.Range.apply in scala.tools.nsc.util.ShowPickled.printFile at pos: 9533


[log inliner] Inlining scala.collection.immutable.Range.inclusive in scala.reflect.internal.util.StatBase.enabled_$eq at pos: 280
[log inliner] Inlining scala.collection.immutable.Range.inclusive in scala.tools.nsc.backend.icode.ICodeCheckers$ICodeChecker.clearStack$1 at pos: 12067
[log inliner] Inlining scala.collection.immutable.Range.inclusive in scala.tools.nsc.backend.opt.InlineExceptionHandlers$InlineExceptionHandlersPhase.scala$tools$nsc$backend$opt$InlineExceptionHandlers$InlineExceptionHandlersPhase$$getTypesAtInstruction at pos: 10911
[log inliner] Inlining scala.collection.immutable.Range.inclusive in scala.tools.nsc.doc.html.page.Template.indentation$1 at pos: 29773


[log inliner] Inlining scala.runtime.RichInt.to in scala.reflect.internal.util.StatBase.enabled_$eq at pos: 280
[log inliner] Inlining scala.runtime.RichInt.to in scala.tools.nsc.backend.icode.ICodeCheckers$ICodeChecker.clearStack$1 at pos: 12067
[log inliner] Inlining scala.runtime.RichInt.to in scala.tools.nsc.backend.opt.InlineExceptionHandlers$InlineExceptionHandlersPhase.scala$tools$nsc$backend$opt$InlineExceptionHandlers$InlineExceptionHandlersPhase$$getTypesAtInstruction at pos: 10911
[log inliner] Inlining scala.runtime.RichInt.to in scala.tools.nsc.doc.html.page.Template.indentation$1 at pos: 29773



On Monday, April 16, 2012 11:36:55 AM UTC+2, Vlad Ureche wrote:

[martin odersky]

On Tue, Apr 24, 2012 at 2:38 AM, Miguel Garcia <miguel...@tuhh.de> wrote:

Hi,

I've made some progress regarding the root cause of the test.stability failure reported in [1]

To find what the inliner does differently, I started by reverting the last band-aid [2] to later compare the -Ylog:inliner output across compiler runs.

That output can be obtained by:

 (1) "ant all.clean build"
 (2) use the resulting compiler and library from build\pack\lib to compile the compiler and library sources.
 (3) run the compiler and library emitted above, on the same sources.

In both cases with "-optimize -Ylog:inliner"

The logs reveal some inlinings occurring in the first but not in the second compilation (I tried also a third compilation, but the logs were identical to those for the second). Details about those inlinings below. They're about Range.{apply, inclusive} and RichInt.to.

"Some progress" because there are at least four pending questions:

  (a) why that happens (macros at play for Range, RichInt?)

I don't think they are used yet in the stdlib.

Cheers

 - Martin

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

[Eugene Burmako]

More precisely, we have two reify macros defined in Context and Universe and not used anywhere, and a bunch of tag materializers defined in reflect.makro.internal in the lib and used by implicit resolution.

On Apr 24, 2012 10:52 AM, "martin odersky" <martin....@epfl.ch> wrote:

On Tue, Apr 24, 2012 at 2:38 AM, Miguel Garcia <miguel...@tuhh.de> wrote:
>
> Hi,
>

> I've made ...

I don't think they are used yet in the stdlib.

Cheers

 - Martin

>
>   (b) a more fine-grain comparison (per classfile) via -Ygen-javap
>

>   (c) going over the l...

[Paul Phillips]

On Mon, Apr 23, 2012 at 5:38 PM, Miguel Garcia <miguel...@tuhh.de> wrote:
>   (d) adopting the policy to 'publicize' less aggressively, allowing it only
> when both callee and caller are defined in the same compilation unit, ie
>         def makePublic(f: Symbol): Boolean =
>           (inc.m.sourceFile ne NoSourceFile)        &&
>           (inc.m.sourceFile == caller.m.sourceFile) &&

It might be enough to check if both caller and callee have source
files; that is, that both are being compiled simultaneously.

[Adriaan Moors]

I tried this (only changing the visibility to public when compiling together)

when I reported the problem (sorry about not sharing my experience),

but the stability problem did not go away

anyway, didn't we agree that we should look for alternatives to making these methods public?

[Miguel Garcia]

More progress on pinpointing the root cause of the instability reported in [1]

An inlining decision takes into account the size of the callee [2], favoring small ones (that's one of several heuristics).

Additionally, there's no topological sorting (over the call-graph relationship) of the methods being visited.

In the case at hand, two different compiler runs result in two different callee bodies (one containing an inlining, unlike the other) arriving at the inliner. The latter body is larger, large enough for the heuristic not to inline that callee.

That has a ripple effect, when the situation above happens for frequent callees. In one case, I'm seeing that for scala.runtime.RichInt.to() where the callee Range$Inclusive.self() was inlined in a run [3] but not in the successive run [4].

Perhaps there are other "instability" sources, and I haven't yet digged to see why [3] inlines but [4] doesn't (that inlining determines whether

  private final int self;

becomes public in the classfile for scala.runtime.RichInt).

As you can see, the root cause is not "making public" per se but whatever leads to the first different inlining decision across runs (which can't be a field made public by a previous inlining, because we're talking about the *first* inlining that goes different).

Barring other differences, from the point of view of stability the inlining differences described above don't pose a risk.

But there's hope: someday we will have a three-address-code based optimizer, designed to be deterministic across runs (but basically more efficient).

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

[1] https://groups.google.com/d/topic/scala-internals/yGmOkBn9Gmk/discussion

[2]   def isSmall       = (length <= SMALL_METHOD_SIZE) && blocks(0).length < 10


[3] compiled by quick:

public scala.collection.immutable.Range$Inclusive to(int, int);
  Code:
   Stack=4, Locals=3, Args_size=3
   0:    getstatic    #21; //Field scala/collection/immutable/Range$.MODULE$:Lscala/collection/immutable/Range$;
   3:    aload_0
   4:    invokevirtual    #23; //Method self:()I
   7:    iload_1
   8:    iload_2
   9:    invokevirtual    #44; //Method scala/collection/immutable/Range$.inclusive:(III)Lscala/collection/immutable/Range$Inclusive;
   12:    areturn

[4] compiled by what quick emits:

public scala.collection.immutable.Range$Inclusive to(int, int);
  Code:
   Stack=5, Locals=4, Args_size=3
   0:    aload_0
   1:    invokevirtual    #23; //Method self:()I
   4:    istore_3
   5:    new    #39; //class scala/collection/immutable/Range$Inclusive
   8:    dup
   9:    iload_3
   10:    iload_1
   11:    iload_2
   12:    invokespecial    #43; //Method scala/collection/immutable/Range$Inclusive."<init>":(III)V
   15:    areturn

[Paul Phillips]

On Tue, Apr 24, 2012 at 4:25 AM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> anyway, didn't we agree that we should look for alternatives to making these
> methods public?

In the meeting martin resisted the idea of trying to finesse the
inliner behavior this late in the game, that's why it's being
revisited.

[Adriaan Moors]

Excellent! That's a very satisfying explanation. --a

[Paul Phillips]

On Tue, Apr 24, 2012 at 5:34 AM, Adriaan Moors <adriaa...@epfl.ch> wrote:
> Excellent! That's a very satisfying explanation. --a

Yeah, that has exactly the "transitive property of influencing
compilation products" we anticipated.

As to what is causing the initial difference: it occurred to me from
something josh said that it could be the order things are fed to
scalac. We know there are bugs which happen with "scalac a.scala
b.scala" but not with "scalac b.scala a.scala", and I imagine there
are similar ones if the classpath isn't given in exactly the same
order, even if it has the same elements on it.

Speaking of that last one, we could always sort the input source files
for scalac before compiling, so that order is deterministic. Pro:
more predictable behavior. Cons: masking bugs; losing a (pretty grim)
"workaround" if the sorted order turns out to be "bug variant" and not
"working variant" for your situation; and the risk of a general
malaise setting in since it feels a little like giving up. Still, it
might be the right thing to do.

[Mark Harrah]

sbt always sorts the input sources in alphabetical order before handing them to scalac for this reason. However, separate compilation can still create situations with symptoms like the ones described here. I don't know if the cause is the same because I suspect unpickling is a confounding variable here.

-Mark