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

Delay delambadficaion and put the lambda's body into the containing class #2887

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@JamesIry
Contributor

JamesIry commented Aug 29, 2013

This is an experimental work in progress PR to bring us closer to using method handles. It will be entirely protected by a flag until it is as solid as the current mechanism.

With the flag enabled the compiler will defer final translation from lambda to class until very late in the pipeline, and it will put the body of the lambda in a method in the original class.

This PR leaves two key things undone

  1. Specialization still needs to be handled.
  2. Logic that partially prevents the use of 'this' in a super constructor call is bypassed. That logic needs to be redone in order to have fewer holes and be less brittle.

James Iry added some commits Jul 22, 2013

James Iry
The toString method on Symbols wasn't properly honoring phase history
which made it hard to use for debugging. This commit makes it
use the symbol's info at the phase it is printing.
James Iry
The future-spec tests were spawning threads in object constructors which
meant they were on the ragged edge of entering deadlock from the static
initialization lock acquired during the static initialization blocks we
use to construct object reference fields. My work on restructuring
lambdas pushed it over the edge. This commit refactors the tests to use
class constructors rather than object constructors.
James Iry
This commit adds a do-nothing phase called "Delambdafy" that will
eventually be responsible for doing the final translation of lambdas
into classes.
James Iry
Refactors Uncurry's transform function to call a generalized
"createMethod" in anticipation of the work to be put into Uncurry for
deferring delambdafication.
James Iry
Adds a setting to delay delambdafication. If set then uncurry lifts
the body of a lambda into a local def. Tests are included to show the
different tree shapes.
James Iry
This commit is purely a refactor. It pulls code needed to adapt a tree
of one type into a tree of another type (by casting, boxing, coercing,
etc) out of Erasure and into common locations that will be usable
from the Delambdafy phase.
James Iry
This commit puts a real body on the Delambdafy phase.
 From a lambda, Delambdafy will create
 1) a static forwarder at the top level of the class that contained
      the lambda
 2) a new top level class that
      a) has fields and a constructor taking the captured environment
(including possbily the "this" reference)
      b) an apply method that calls the static forwarder
      c) if needed a bridge method for the apply method
 3) an instantiation of the newly created class which replaces the
   lambda

A few basic tests are included to verify that it works as expected.
Further commits will have additional tests.
James Iry
During development of delayed delambdafy there was a problem where
GenASM would eliminate a loadmodule for all methods defined within that
module even if those methods were static. The result would be broken
byte code that failed verification. This commit fixes that and adds a
test.
James Iry
During development of late delmabdafying there was a problem where
specialization would undo some of the work done in uncurry if the body
of the lambda had a constant type. That would result in a compiler crash
as when the delambdafy phase got a tree shape it didn't understand.
This commit has a fix and a test.
James Iry
This commit includes several tests where there's a variation in
signatures between inline delambdafication and method based
delambdafication.
James Iry
This test forces several tests to run using inline delambdafication.
The differences when running with method based delambdafication aren't
important enough yet to create specialized versions that use method
based delambdafication.
James Iry
These tests represent a big TODO. We currently have a bit of logic that
attempts to protect the compiler from generating byte code that uses
'this' during a call to a super constructor. There are two problems with
that code. First, it happens early before delambdafication has a chance
to simplify the code. Second, it's incomplete and it's easy to write
code that sneaks past the verification but produces broken byte code.
The conclusion from the team is that that logic needs to be pushed into
a much later phase, possibly after going to the ICode/ASM IR. But that
change is outside the scope of this work so for now these tests are
simply forced to run only with inline delambdafication.
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 29, 2013

Member

🍰!

Member

adriaanm commented Aug 29, 2013

🍰!

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Deconsting defensively, or to make something work (perhaps later in the commit sequence?) How could we get a X => 1.type? We shouldn't infer it. I suppose a macro could conjure one.

Deconsting defensively, or to make something work (perhaps later in the commit sequence?) How could we get a X => 1.type? We shouldn't infer it. I suppose a macro could conjure one.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

* and => are usually bug bedfellows.

Here's another test case:

scala> ((x => 0): ((=> Any) => Int)).apply(sys.error("!!"))
res1: Int = 0

* and => are usually bug bedfellows.

Here's another test case:

scala> ((x => 0): ((=> Any) => Int)).apply(sys.error("!!"))
res1: Int = 0
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Just as a public-service-announcement, as its not a big win right here, but we've also got the sm interpolator for margin stripping and interpolating all at once.

retronym commented on test/files/run/uncurry_inline.scala in 52cdea1 Aug 29, 2013

Just as a public-service-announcement, as its not a big win right here, but we've also got the sm interpolator for margin stripping and interpolating all at once.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

We ought to also test * (with and without -Yeta-expand-keeps-star), and => as I've noted above.

retronym commented on test/files/run/uncurry_method.scala in 52cdea1 Aug 29, 2013

We ought to also test * (with and without -Yeta-expand-keeps-star), and => as I've noted above.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

I didn't know about replace, handy.

In this case I would have written treeCopy.Function(fun, fun.vparams, liftedMethodCall), which will be a bit more efficient.

I didn't know about replace, handy.

In this case I would have written treeCopy.Function(fun, fun.vparams, liftedMethodCall), which will be a bit more efficient.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

I'd factor out target.symbol.isLocal && target.symbol.isArtifact && target.symbol.name.containsName(nme.ANON_FUN_NAME) into a method.

I'd factor out target.symbol.isLocal && target.symbol.isArtifact && target.symbol.name.containsName(nme.ANON_FUN_NAME) into a method.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Might as well add nme. ANON_FUN_NAME. Or maybe pick a new variation for these methods.

Might as well add nme. ANON_FUN_NAME. Or maybe pick a new variation for these methods.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Member

Commit comments need a little brush up to follow the "Subject... \n\nBody" convention.

Member

retronym commented Aug 29, 2013

Commit comments need a little brush up to follow the "Subject... \n\nBody" convention.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Maybe lose -Transformer from the name. I was looking for the transform method.

Maybe lose -Transformer from the name. I was looking for the transform method.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Although I'm not the biggest fan, the rest of the compiler tends for EmptyTree rather than None, so it might be best to toe the line.

Although I'm not the biggest fan, the rest of the compiler tends for EmptyTree rather than None, so it might be best to toe the line.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Give us a message :)

Give us a message :)

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

An example in this comment would help me understand this.

An example in this comment would help me understand this.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

I wonder if you need to consider Super, too?

I wonder if you need to consider Super, too?

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

Is there a chance that this name will clash with a name in a sub- or super-class?

Is there a chance that this name will clash with a name in a sub- or super-class?

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 30, 2013

object Main {
  def g(a: => Any) = ()
  def main(args: Array[String]) {
    g(0)
  }
  def accessor1(): Int = ???
}

[[syntax trees at end of                delambdafy]] // test.scala
package <empty> {
  object Main extends Object {
    def g(a: Function0): Unit = ();
    def main(args: Array[String]): Unit = Main.this.g({
      (new Main$lambda$main$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
    });
    def accessor1(): Int = scala.this.Predef.???();
    final <static> <artifact> private[this] def $anonfun$1(): Int = 0;
    def <init>(): Main.type = {
      Main.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = Main.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class Main$lambda$main$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): Main$lambda$main$1 = {
      Main$lambda$main$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = Main.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(Main$lambda$main$1.this.apply())
  }
}

java.lang.ClassFormatError: Duplicate method name&signature in class file Main$
    at java.lang.ClassLoader.defineClass1(Native Method)

So for starters, we need a '$' in the accessor name.

But even with that, it is only unique in this compilation unit, so it could clash with another generated accessor$N method in a superclass.

I couldn't make this trigger the wrong behaviour, but here's what I tried:

 tail  sandbox/{T,test}.scala
==> sandbox/T.scala <==
class T {
  private def f(a: () => Int) = {
    val v1 = a()
    println(s"T#f($v1)")
  }
  def foo {
    f(() => 0)
  }
}


==> sandbox/test.scala <==
object Main extends T {
  private def g(a: () => Int) = println(s"Main.g(${a()})")
  def main(args: Array[String]) {
    foo
    g(() => 1)
  }
}

2887 /code/scala qbin/scalac -Ydelambdafy:method -Xprint:delambdafy sandbox/{T,test}.scala && qbin/scala Main
[[syntax trees at end of                delambdafy]] // T.scala
package <empty> {
  class T extends Object {
    private def f(a: Function0): Unit = {
      val v1: Int = a.apply$mcI$sp();
      scala.this.Predef.println(new StringContext(scala.this.Predef.wrapRefArray(Array[String]{"T#f(", ")"}.$asInstanceOf[Array[Object]]())).s(scala.this.Predef.genericWrapArray(Array[Object]{scala.Int.box(v1)})))
    };
    def foo(): Unit = T.this.f({
      (new T$lambda$foo$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
    });
    final <static> <artifact> private[this] def $anonfun$1(): Int = 0;
    def <init>(): T = {
      T.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = T.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class T$lambda$foo$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): T$lambda$foo$1 = {
      T$lambda$foo$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = T.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(T$lambda$foo$1.this.apply())
  }
}

 // test.scala
package <empty> {
  object Main extends T {
    private def g(a: Function0): Unit = scala.this.Predef.println(new StringContext(scala.this.Predef.wrapRefArray(Array[String]{"Main.g(", ")"}.$asInstanceOf[Array[Object]]())).s(scala.this.Predef.genericWrapArray(Array[Object]{scala.Int.box(a.apply$mcI$sp())})));
    def main(args: Array[String]): Unit = {
      Main.this.foo();
      Main.this.g({
        (new Main$lambda$main$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
      })
    };
    final <static> <artifact> private[this] def $anonfun$1(): Int = 1;
    def <init>(): Main.type = {
      Main.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = Main.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class Main$lambda$main$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): Main$lambda$main$1 = {
      Main$lambda$main$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = Main.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(Main$lambda$main$1.this.apply())
  }
}

T#f(0)
Main.g(1)

retronym replied Aug 30, 2013

object Main {
  def g(a: => Any) = ()
  def main(args: Array[String]) {
    g(0)
  }
  def accessor1(): Int = ???
}

[[syntax trees at end of                delambdafy]] // test.scala
package <empty> {
  object Main extends Object {
    def g(a: Function0): Unit = ();
    def main(args: Array[String]): Unit = Main.this.g({
      (new Main$lambda$main$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
    });
    def accessor1(): Int = scala.this.Predef.???();
    final <static> <artifact> private[this] def $anonfun$1(): Int = 0;
    def <init>(): Main.type = {
      Main.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = Main.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class Main$lambda$main$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): Main$lambda$main$1 = {
      Main$lambda$main$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = Main.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(Main$lambda$main$1.this.apply())
  }
}

java.lang.ClassFormatError: Duplicate method name&signature in class file Main$
    at java.lang.ClassLoader.defineClass1(Native Method)

So for starters, we need a '$' in the accessor name.

But even with that, it is only unique in this compilation unit, so it could clash with another generated accessor$N method in a superclass.

I couldn't make this trigger the wrong behaviour, but here's what I tried:

 tail  sandbox/{T,test}.scala
==> sandbox/T.scala <==
class T {
  private def f(a: () => Int) = {
    val v1 = a()
    println(s"T#f($v1)")
  }
  def foo {
    f(() => 0)
  }
}


==> sandbox/test.scala <==
object Main extends T {
  private def g(a: () => Int) = println(s"Main.g(${a()})")
  def main(args: Array[String]) {
    foo
    g(() => 1)
  }
}

2887 /code/scala qbin/scalac -Ydelambdafy:method -Xprint:delambdafy sandbox/{T,test}.scala && qbin/scala Main
[[syntax trees at end of                delambdafy]] // T.scala
package <empty> {
  class T extends Object {
    private def f(a: Function0): Unit = {
      val v1: Int = a.apply$mcI$sp();
      scala.this.Predef.println(new StringContext(scala.this.Predef.wrapRefArray(Array[String]{"T#f(", ")"}.$asInstanceOf[Array[Object]]())).s(scala.this.Predef.genericWrapArray(Array[Object]{scala.Int.box(v1)})))
    };
    def foo(): Unit = T.this.f({
      (new T$lambda$foo$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
    });
    final <static> <artifact> private[this] def $anonfun$1(): Int = 0;
    def <init>(): T = {
      T.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = T.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class T$lambda$foo$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): T$lambda$foo$1 = {
      T$lambda$foo$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = T.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(T$lambda$foo$1.this.apply())
  }
}

 // test.scala
package <empty> {
  object Main extends T {
    private def g(a: Function0): Unit = scala.this.Predef.println(new StringContext(scala.this.Predef.wrapRefArray(Array[String]{"Main.g(", ")"}.$asInstanceOf[Array[Object]]())).s(scala.this.Predef.genericWrapArray(Array[Object]{scala.Int.box(a.apply$mcI$sp())})));
    def main(args: Array[String]): Unit = {
      Main.this.foo();
      Main.this.g({
        (new Main$lambda$main$1(): runtime.AbstractFunction0).$asInstanceOf[Function0]()
      })
    };
    final <static> <artifact> private[this] def $anonfun$1(): Int = 1;
    def <init>(): Main.type = {
      Main.super.<init>();
      ()
    };
    final <synthetic> <static> <bridge> protected def accessor1(): Int = Main.this.$anonfun$1()
  };
  @SerialVersionUID(0) final <synthetic> class Main$lambda$main$1 extends runtime.AbstractFunction0 with Serializable {
    <synthetic> def <init>(): Main$lambda$main$1 = {
      Main$lambda$main$1.super.<init>();
      ()
    };
    final <synthetic> def apply(): Int = Main.this.accessor1();
    final <synthetic> <bridge> def apply(): Object = scala.Int.box(Main$lambda$main$1.this.apply())
  }
}

T#f(0)
Main.g(1)
@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 29, 2013

If we do push it back, the trick will be to give decently positioned and meaningful error messages.

retronym commented on bf1ba25 Aug 29, 2013

If we do push it back, the trick will be to give decently positioned and meaningful error messages.

@huitseeker

This comment has been minimized.

Show comment
Hide comment
@huitseeker

huitseeker Aug 29, 2013

Contributor

The multiple fails of the validator on this PR, e.g. this IDE failure on 52cdea193ee91301197b40823ac7a9354bd60a14
are genuine.

Contributor

huitseeker commented Aug 29, 2013

The multiple fails of the validator on this PR, e.g. this IDE failure on 52cdea193ee91301197b40823ac7a9354bd60a14
are genuine.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 30, 2013

Member

This patch almost gets this test working again. (Same number of errors but different messages.)

I know this stuff is pretty hacky, but it might help until we figure out the best approach.

diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/L
index ce495ca..1b0d371 100644
--- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
+++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
@@ -342,7 +342,12 @@ abstract class LambdaLift extends InfoTransform {
         currentUnit.error(curTree.pos, msg)
       }
       val qual =
-        if (clazz == currentClass) gen.mkAttributedThis(clazz)
+        if (clazz == currentClass) {
+          if (currentOwner.isMethod && currentOwner.name.startsWith(tpnme.ANON_FUN_NAME) && isUnderConstructio
+            prematureSelfReference()
+            EmptyTree
+          } else gen.mkAttributedThis(clazz)
+        }
         else {
Member

retronym commented on test/files/neg/t6666.flags in bf1ba25 Aug 30, 2013

This patch almost gets this test working again. (Same number of errors but different messages.)

I know this stuff is pretty hacky, but it might help until we figure out the best approach.

diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/L
index ce495ca..1b0d371 100644
--- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
+++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
@@ -342,7 +342,12 @@ abstract class LambdaLift extends InfoTransform {
         currentUnit.error(curTree.pos, msg)
       }
       val qual =
-        if (clazz == currentClass) gen.mkAttributedThis(clazz)
+        if (clazz == currentClass) {
+          if (currentOwner.isMethod && currentOwner.name.startsWith(tpnme.ANON_FUN_NAME) && isUnderConstructio
+            prematureSelfReference()
+            EmptyTree
+          } else gen.mkAttributedThis(clazz)
+        }
         else {
@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Sep 2, 2013

Member

I couldn't resist, so I hacked this:
sjrd/scala-js@3c913fa
over the last 2.5 hours.
Support for -Ydelambdafy:method for Scala.js, based on this pull request.
Seems to work, but I'm too tired to really reason about it thoroughly. I did compile the entire standard library + the reversi demo with -Ydelambdafy:method, and it worked well :)

Basically there is one concrete class per arity, e.g.,

final class AnonFunction1[-T1, +R](f: js.Function1[T1, R]) extends AbstractFunction1[T1, R] {
  override def apply(arg1: T1): R = f(arg1)
}

and all anonymous functions are simply translated into an instantiation of the appropriate AnonFunctionN with a JavaScript lambda as parameter.

@JamesIry From your code and from output trees, I inferred the following:

Anonymous functions survive until the backend only under
-Ydelambdafy:method
and when they do, their body is always of the form
EnclosingClass.this.someMethod(arg1, ..., argN, capture1, ..., captureM)
where argI are the formal arguments of the lambda, and captureI are local variables or the enclosing def.

Is it indeed true? Did I get it right?

Member

sjrd commented Sep 2, 2013

I couldn't resist, so I hacked this:
sjrd/scala-js@3c913fa
over the last 2.5 hours.
Support for -Ydelambdafy:method for Scala.js, based on this pull request.
Seems to work, but I'm too tired to really reason about it thoroughly. I did compile the entire standard library + the reversi demo with -Ydelambdafy:method, and it worked well :)

Basically there is one concrete class per arity, e.g.,

final class AnonFunction1[-T1, +R](f: js.Function1[T1, R]) extends AbstractFunction1[T1, R] {
  override def apply(arg1: T1): R = f(arg1)
}

and all anonymous functions are simply translated into an instantiation of the appropriate AnonFunctionN with a JavaScript lambda as parameter.

@JamesIry From your code and from output trees, I inferred the following:

Anonymous functions survive until the backend only under
-Ydelambdafy:method
and when they do, their body is always of the form
EnclosingClass.this.someMethod(arg1, ..., argN, capture1, ..., captureM)
where argI are the formal arguments of the lambda, and captureI are local variables or the enclosing def.

Is it indeed true? Did I get it right?

@magarciaEPFL

This comment has been minimized.

Show comment
Hide comment
@magarciaEPFL

magarciaEPFL Sep 3, 2013

Contributor

The point raised by @sjrd is relevant for future compatibility with JDK 8, quoting from magarciaEPFL@2cee8fd

A LambdaMetaFactory in JDK8 delivers lambdas that assume
the (hoisted) target method to have arguments for all captured values and for apply-args (in that order).
In contrast, "hoisted target methods" emerge from LambdaLift with formal-params for captured values appended after those for apply-args.
Moreover, for mixins, the first formal of an endpoint in an implementation class denotes the self-reference.

Contributor

magarciaEPFL commented Sep 3, 2013

The point raised by @sjrd is relevant for future compatibility with JDK 8, quoting from magarciaEPFL@2cee8fd

A LambdaMetaFactory in JDK8 delivers lambdas that assume
the (hoisted) target method to have arguments for all captured values and for apply-args (in that order).
In contrast, "hoisted target methods" emerge from LambdaLift with formal-params for captured values appended after those for apply-args.
Moreover, for mixins, the first formal of an endpoint in an implementation class denotes the self-reference.

@magarciaEPFL

This comment has been minimized.

Show comment
Hide comment
@magarciaEPFL

magarciaEPFL Sep 3, 2013

Contributor

Any preliminary benchmark on how much faster the compiler compiles under -Ydelambdafy:method ? Yes, I know no benchmark is perfect, but the following helps: compile the compiler and let us know the wall-clock time.

Contributor

magarciaEPFL commented Sep 3, 2013

Any preliminary benchmark on how much faster the compiler compiles under -Ydelambdafy:method ? Yes, I know no benchmark is perfect, but the following helps: compile the compiler and let us know the wall-clock time.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Sep 3, 2013

Member

A LambdaMetaFactory in JDK8 delivers lambdas that assume the (hoisted) target method to have arguments for all captured values and for apply-args (in that order).

As a matter of fact, it is also the ordering best supported by Function.prototype.bind in JavaScript.
Given that both systems would be "more comfortable" with captures before, I suggest that delambdafy follow that scheme.

Member

sjrd commented Sep 3, 2013

A LambdaMetaFactory in JDK8 delivers lambdas that assume the (hoisted) target method to have arguments for all captured values and for apply-args (in that order).

As a matter of fact, it is also the ordering best supported by Function.prototype.bind in JavaScript.
Given that both systems would be "more comfortable" with captures before, I suggest that delambdafy follow that scheme.

@magarciaEPFL

This comment has been minimized.

Show comment
Hide comment
@magarciaEPFL

magarciaEPFL Sep 3, 2013

Contributor

This PR contains several commits that are refactorings or fixes unrelated to "delamb". Perhaps it's best to factor them out into their own PR, review them, merge (they should be far less controversial). A preliminary list of candidates follows.

Looks like each of the following doesn't depend on any other.

The following one is also independent of the previous ones but with a "delamb" smell to it:

That leaves only six other commits as the real payload of this PR.

HTH!

Contributor

magarciaEPFL commented Sep 3, 2013

This PR contains several commits that are refactorings or fixes unrelated to "delamb". Perhaps it's best to factor them out into their own PR, review them, merge (they should be far less controversial). A preliminary list of candidates follows.

Looks like each of the following doesn't depend on any other.

The following one is also independent of the previous ones but with a "delamb" smell to it:

That leaves only six other commits as the real payload of this PR.

HTH!

@JamesIry

This comment has been minimized.

Show comment
Hide comment
@JamesIry

JamesIry Sep 3, 2013

Contributor

Thanks for the feedback everyone.

@retronym I've got a fix for the test failure. It has to do with self types vs inheritance.
@magarciaEPFL I'll pull the Symbol.toString commit into a different PR. All the others are directly related to the story of the PR in the sense that delayed delambdafication doesn't work without the previous steps.
@sjrd I'll reorder the arguments with the next iteration of this PR.

Contributor

JamesIry commented Sep 3, 2013

Thanks for the feedback everyone.

@retronym I've got a fix for the test failure. It has to do with self types vs inheritance.
@magarciaEPFL I'll pull the Symbol.toString commit into a different PR. All the others are directly related to the story of the PR in the sense that delayed delambdafication doesn't work without the previous steps.
@sjrd I'll reorder the arguments with the next iteration of this PR.

@magarciaEPFL

This comment has been minimized.

Show comment
Hide comment
@magarciaEPFL

magarciaEPFL Sep 3, 2013

Contributor

@JamesIry

All the others are directly related to the story of the PR in the sense that delayed delambdafication doesn't work without the previous steps.

Yes, I know. The suggestion to merge them earlier was motivated because in general I'm in favor of merging sthg as soon as it's known to work well. For example, commit JamesIry@3ce5c97 is useful on its own.

I guess as long as this PR doesn't take that long to review it's ok to keep them bundled. Otherwise those commits "useful on their own" should be merged earlier.

Contributor

magarciaEPFL commented Sep 3, 2013

@JamesIry

All the others are directly related to the story of the PR in the sense that delayed delambdafication doesn't work without the previous steps.

Yes, I know. The suggestion to merge them earlier was motivated because in general I'm in favor of merging sthg as soon as it's known to work well. For example, commit JamesIry@3ce5c97 is useful on its own.

I guess as long as this PR doesn't take that long to review it's ok to keep them bundled. Otherwise those commits "useful on their own" should be merged earlier.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 6, 2013

Member

Temptingly close, but alas! This will slip to M6.

Member

adriaanm commented Sep 6, 2013

Temptingly close, but alas! This will slip to M6.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 21, 2013

Member

The build kitteh was misbehaving and jobs from this PR were being built over and over. I've tried to manually cancel them in Jenkins to clear the congestion. Please disregard the latest build failures.

Member

retronym commented Sep 21, 2013

The build kitteh was misbehaving and jobs from this PR were being built over and over. I've tried to manually cancel them in Jenkins to clear the congestion. Please disregard the latest build failures.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 21, 2013

Member

NOLITTER!

Member

retronym commented Sep 21, 2013

NOLITTER!

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

(kitty-note-to-self: ignore 24857746)
🐱 cleaning up... sorry! 🐱

scala-jenkins commented Sep 21, 2013

(kitty-note-to-self: ignore 24857746)
🐱 cleaning up... sorry! 🐱

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for 3ce5c97 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@3ce5c97d074f7b98d3740afcd95cc970eaf8da95" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on 3ce5c97 Sep 21, 2013

Job pr-scala failed for 3ce5c97 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@3ce5c97d074f7b98d3740afcd95cc970eaf8da95" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for f77ddaf Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@f77ddaf3768912dfee9ce67b075243fb5410677e" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on f77ddaf Sep 21, 2013

Job pr-scala failed for f77ddaf Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@f77ddaf3768912dfee9ce67b075243fb5410677e" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for e6125a4 Took 5 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@e6125a456fc73afd6f1bdcf79758fab1286bc060" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on e6125a4 Sep 21, 2013

Job pr-scala failed for e6125a4 Took 5 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@e6125a456fc73afd6f1bdcf79758fab1286bc060" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for 15ef90d Took 5 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@15ef90d01b3feb10260fad6fa00cea3c12f342de" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on 15ef90d Sep 21, 2013

Job pr-scala failed for 15ef90d Took 5 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@15ef90d01b3feb10260fad6fa00cea3c12f342de" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for 52cdea1 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@52cdea193ee91301197b40823ac7a9354bd60a14" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on 52cdea1 Sep 21, 2013

Job pr-scala failed for 52cdea1 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@52cdea193ee91301197b40823ac7a9354bd60a14" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for 8411cd3 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@8411cd3e9e2de1d4f6e3d6c08d89fd6fa00f8c45" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on 8411cd3 Sep 21, 2013

Job pr-scala failed for 8411cd3 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@8411cd3e9e2de1d4f6e3d6c08d89fd6fa00f8c45" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for 35033ad Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@35033ad0bb4339d309558e24d44f14ab970f8802" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on 35033ad Sep 21, 2013

Job pr-scala failed for 35033ad Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@35033ad0bb4339d309558e24d44f14ab970f8802" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@scala-jenkins

This comment has been minimized.

Show comment
Hide comment
@scala-jenkins

scala-jenkins Sep 21, 2013

Job pr-scala failed for bf1ba25 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@bf1ba254a46fe81e3015901d527b1e8677e4050e" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

scala-jenkins commented on bf1ba25 Sep 21, 2013

Job pr-scala failed for bf1ba25 Took 4 min. (results):


To retry exactly this commit (if the failure was spurious), comment "PLS REBUILD/pr-scala@bf1ba254a46fe81e3015901d527b1e8677e4050e" on PR 2887.NOTE: new commits are rebuilt automatically as they appear. There's no need to force a rebuild if you're updating the PR.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Sep 23, 2013

Member

Closing in part due to the merge conflicts and in part due to the fact the build kitteh is in an endless build loop at the moment and this PR has been built a dozen times over the weekend.

Member

retronym commented Sep 23, 2013

Closing in part due to the merge conflicts and in part due to the fact the build kitteh is in an endless build loop at the moment and this PR has been built a dozen times over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment