Use LambdaMetafactory where possible for lambda creation.#4463
Use LambdaMetafactory where possible for lambda creation.#4463lrytz merged 6 commits intoscala:2.11.xfrom
Conversation
Suitable lambdas are identified in Delambdafy and marked with such with a tree annotation that includes the data needed by the backend to emit an invokedynamic instruction. GenBCode to rewrite instantiation of such anonymous function classes with an invokedynamic instruction. At this stage, I don't plan to merge the support for this into GenASM. Between these points, the lambda capture is represented as an application of a dummy factory symbol: ``` <dummy>(captures...) : FunctionN ``` Demo: ``` % wget http://central.maven.org/maven2/org/scala-lang/modules/scala-java8-compat_2.11/0.3.0/scala-java8-compat_2.11-0.3.0.jar % qscala -classpath scala-java8-compat_2.11-0.3.0.jar -Ydelambdafy:method -target:jvm-1.8 -Ybackend:GenBCode Welcome to Scala version 2.11.6-20150309-144147-c91c978c81 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_25). Type in expressions to have them evaluated. Type :help for more information. scala> (() => "").getClass res0: Class[_ <: () => String] = class $$Lambda$1/871790326 ``` I have also corrected an error in a previous commit. The newly added symbol test, `isDelambdafyTarget`, needs to check for the `ARTIFACT` flag, as that is what is added to the method by `Uncurry`.
When the ant build is run with `-Dscala-java8-compat.package`, the contents of `org.scala-lang.modules:scala-java8-compat` are spliced into `scala-library.jar`. This is handy to facilitate downstream testing of indy lambdas, we can just use the the compiler options `-target 1.8 -Ydelambdafy:method -Xexperimental` without needed to add `scala-java8-compat.jar` to the compile and runtime classpaths. (Classpath augmentation doesn't appear to be straight forward in partest or in the community build.)
```
scala> (x: Int) => {??? : Int}
res2: Int => Int = $$Lambda$1371/1961176822@6ed3ccb2
scala> res2(42)
scala.NotImplementedError: an implementation is missing
at scala.Predef$.$qmark$qmark$qmark(Predef.scala:225)
at .$anonfun$1(<console>:8)
at $$Lambda$1371/1961176822.apply$mcII$sp(Unknown Source)
... 33 elided
scala> (x: Int, y: Long) => {??? : Int}
res4: (Int, Long) => Int = $$Lambda$1382/1796047085@6f8e8894
scala> res4(0, 0L)
scala.NotImplementedError: an implementation is missing
at scala.Predef$.$qmark$qmark$qmark(Predef.scala:225)
at .$anonfun$1(<console>:8)
at $$Lambda$1382/1796047085.apply$mcIIJ$sp(Unknown Source)
... 33 elided
```
|
While many of these the failures listed in the PR description are benign, there is at least one problematic one I'm investigating: |
|
That one minimizes to: object Test {
def main(args: Array[String]): Unit = {
val f: (Int, => Int) => Int = (x, y) => 0
f(null.asInstanceOf[Int], 0)
}
} |
|
Okay, this could be a problem. We had assumed that scala> Int.unbox(null)
res1: Int = 0
scala> val i2i = (x: Int) => x + 1
i2i: Int => Int = <function1>
scala> i2i.asInstanceOf[AnyRef => Int]
res4: AnyRef => Int = <function1>
scala> res4(null)
res5: Int = 1 |
There was a problem hiding this comment.
What't the reason for this new definition? There seems to be only the one callsite above.
There was a problem hiding this comment.
IT does seem unnecessary (albeit not harmful) now, perhaps I had another caller during an earlier iteration of the patch.
|
looks very good! |
|
If we are unable to use the unboxing synthesized by LMF (which is just a virtual call to val f = (s: String) => 42
// def $target(s: String): Integer = Int.box(42)
// val f = indy(LambdaMetaFactory, <$target>) |
|
Would we lose specialization of functions in this approach? |
|
With a specialized function, the abstract method already deals in primitives, so |
|
A tradeoff is that we might incur unwanted boxing when the lambda target method is directly called from some Scala call site as a result of inlining, rather than being called through the generic interfaces method. e.g. def inlineMe = ((i: Int) => (i + 1).toString)(42)
def $lambdaTarget(i: Integer): String = Int.box((Int.unbox(i) + 1)).toString
def inlineMeOptimized = $lambdaTarget(Int.box(42))In cases when we inline the lambda target as well, this peephole optimizations might get rid of the box/unbox pairs. |
|
OK, got it. One idea for the inlining issue (might be too much overhead though): we could generate the lambda target methods as they are now, i.e. without boxing. Then we could have a second method that only boxes and delegates, and use this as the actual target for the metafactory. This way, after inlining the lambda, the optimizer could rewrite It would also make the workaround a bit more self-contained - there might be multiple places in the lambda target where boxing / unboxing has to be introduced (multiple return points). |
|
To put this into context, I believe you need to cast to notice the NPE-on-unboxing issue. So as long as we think it is solvable, we could tackle it in a followup. E.g. in the worst case, a clumsy but correct solution would be to use the old translation for non-specialized function types that deal in primitive value types. In the meantime, it would be good to establish a foothold to start work on the inliner and serialization |
|
Indirection through another method is certainly possible. Would be similar going through the static accessor (which I used to use for argument permutation.) We could teach the inliner to always inline that method, as I think it would always be beneficial. OTOH, we should seek to have the most direct call path when possible, so we might use this only in problematic cases. |
|
What is the situation with the void / BoxedUnit issue? |
|
Super! Could you distill the boxing snags encountered and resolved in the design doc? Maybe illuminate our current choice and future refinements in terms of an overview of possible solutions? |
|
FWIW, here's how I dealt with LMF and value classes in Dotty: smarter/dotty@48af911 (part of scala/scala3#411, Dotty always uses LMF for closures, so I had to implement something). This generates a "bridge" method which does the boxing/unboxing and calls the method which actually implements the closure. Also, good catch with that null unboxing issue, supporting this is going to be annoying. |
|
@lrytz Currently we solve the This was feasible as it doesn't add an explosion of functional interfaces: we just need an additional 22. However, we also could solve this by pushing this into the lambda target method, as proposed above for numeric box/unboxing.
|
|
✨ |
|
Here's a spike at the DIY boxing approach: retronym/scala@topic/indylambda-emit-indy...retronym:topic/indylambda-diy-boxing |
|
that can go into a subsequent PR, no? |
|
Yes, I think it would add too much bulk to this one to try to include that. |
|
It's a bit unfortunate that we're not yet building on jdk 8, so we can't really have tests yet. Once a change is merged, the motivation to write tests for it usually declines :-) |
|
I guess I ought to write the tests but leave them unmerged until we land on 2.12.x. Or we could make them noops on Java 6/7 but assert something on Java 8... |
There was a problem hiding this comment.
If target is non-static you should also load the receiver reference. I do not see where you do it.
There was a problem hiding this comment.
In that case, Delambdafy rewrites to $anonFunFactory(receiver, arg0, ... argN), where $anonFunFactory is a fictional symbol with a result type (LambdaParam0, .. LambdaParam0) => LambdaResult
|
LGTM! |
Use LambdaMetafactory where possible for lambda creation.
`LambdaMetafactory` generates code to perform a limited number
of type adaptations when delegating from its implementation of
the functional interface method to the lambda target method.
These adaptations are: numeric widening, casting, boxing and unboxing.
However, the semantics of unboxing numerics in Java differs to Scala:
they treat `UNBOX(null)` as cause to rause a `NullPointerException`,
Scala (in `BoxesRuntime.unboxTo{Byte,Short,...}`) reinterprets the
null as zero.
Furthermore, Java has no idea how to adapt between a value class and
its wrapped type, nor from a void return to `BoxedUnit`.
This commit detects when the lambda target method would require
such adaptation. If it does, an extra method, `$anonfun$1$adapted` is
created to perform the adaptation, and this is used as the target
of the lambda.
This obviates the use of `JProcedureN` for `Unit` returning
lambdas, we know use `JFunctionN` as the functional interface
and bind this to an `$adapted` method that summons the instance
of `BoxedUnit` after calling the `void` returning lambda target.
The enclosed test cases fail without boxing changes. They don't
execute with indylambda enabled under regular partest runs yet,
you need to add scala-java8-compat to scala-library and pass
the SCALAC_OPTS to partest manually to try this out, as described
in scala#4463. Once we enable indylambda
by default, however, this test will exercise the code in this patch
all the time.
It is also possible to run the tests with:
```
% curl https://oss.sonatype.org/content/repositories/releases/org/scala-lang/modules/scala-java8-compat_2.11/0.4.0/scala-java8-compat_2.11-0.4.0.jar > scala-java8-compat_2.11-0.4.0.jar
% export INDYLAMBDA="-Ydelambdafy:method -Ybackend:GenBCode -target:jvm-1.8 -classpath .:scala-java8-compat_2.11-0.4.0.jar"
qscalac $INDYLAMBDA test/files/run/indylambda-boxing/*.scala && qscala $INDYLAMBDA Test
```
`LambdaMetafactory` generates code to perform a limited number
of type adaptations when delegating from its implementation of
the functional interface method to the lambda target method.
These adaptations are: numeric widening, casting, boxing and unboxing.
However, the semantics of unboxing numerics in Java differs to Scala:
they treat `UNBOX(null)` as cause to rause a `NullPointerException`,
Scala (in `BoxesRuntime.unboxTo{Byte,Short,...}`) reinterprets the
null as zero.
Furthermore, Java has no idea how to adapt between a value class and
its wrapped type, nor from a void return to `BoxedUnit`.
This commit detects when the lambda target method would require
such adaptation. If it does, an extra method, `$anonfun$1$adapted` is
created to perform the adaptation, and this is used as the target
of the lambda.
This obviates the use of `JProcedureN` for `Unit` returning
lambdas, we know use `JFunctionN` as the functional interface
and bind this to an `$adapted` method that summons the instance
of `BoxedUnit` after calling the `void` returning lambda target.
The enclosed test cases fail without boxing changes. They don't
execute with indylambda enabled under regular partest runs yet,
you need to add scala-java8-compat to scala-library and pass
the SCALAC_OPTS to partest manually to try this out, as described
in scala#4463. Once we enable indylambda
by default, however, this test will exercise the code in this patch
all the time.
It is also possible to run the tests with:
```
% curl https://oss.sonatype.org/content/repositories/releases/org/scala-lang/modules/scala-java8-compat_2.11/0.4.0/scala-java8-compat_2.11-0.4.0.jar > scala-java8-compat_2.11-0.4.0.jar
% export INDYLAMBDA="-Ydelambdafy:method -Ybackend:GenBCode -target:jvm-1.8 -classpath .:scala-java8-compat_2.11-0.4.0.jar"
qscalac $INDYLAMBDA test/files/run/indylambda-boxing/*.scala && qscala $INDYLAMBDA Test
```
`LambdaMetafactory` generates code to perform a limited number
of type adaptations when delegating from its implementation of
the functional interface method to the lambda target method.
These adaptations are: numeric widening, casting, boxing and unboxing.
However, the semantics of unboxing numerics in Java differs to Scala:
they treat `UNBOX(null)` as cause to raise a `NullPointerException`,
Scala (in `BoxesRuntime.unboxTo{Byte,Short,...}`) reinterprets the
null as zero.
Furthermore, Java has no idea how to adapt between a value class and
its wrapped type, nor from a void return to `BoxedUnit`.
This commit detects when the lambda target method would require
such adaptation. If it does, an extra method, `$anonfun$1$adapted` is
created to perform the adaptation, and this is used as the target
of the lambda.
This obviates the use of `JProcedureN` for `Unit` returning
lambdas, we know use `JFunctionN` as the functional interface
and bind this to an `$adapted` method that summons the instance
of `BoxedUnit` after calling the `void` returning lambda target.
The enclosed test cases fail without boxing changes. They don't
execute with indylambda enabled under regular partest runs yet,
you need to add scala-java8-compat to scala-library and pass
the SCALAC_OPTS to partest manually to try this out, as described
in scala#4463. Once we enable indylambda
by default, however, this test will exercise the code in this patch
all the time.
It is also possible to run the tests with:
```
% curl https://oss.sonatype.org/content/repositories/releases/org/scala-lang/modules/scala-java8-compat_2.11/0.4.0/scala-java8-compat_2.11-0.4.0.jar > scala-java8-compat_2.11-0.4.0.jar
% export INDYLAMBDA="-Ydelambdafy:method -Ybackend:GenBCode -target:jvm-1.8 -classpath .:scala-java8-compat_2.11-0.4.0.jar"
qscalac $INDYLAMBDA test/files/run/indylambda-boxing/*.scala && qscala $INDYLAMBDA Test
```
My design notes: https://gist.github.com/retronym/0178c212e4bacffed568
This is all locked behind:
We require scala-java8-compat to be present on the compilation and runtime classpath. This provides a set of functional interfaces.
These are automatically downloaded and added to
scala-library.jarif the propertyscala-java8-compat.packageis set during the Ant build. This option is only intended to facilitate testing, we don't plan to ship those classes in any Scala release.We can bootstrap with this enabled:
Limitations
LambdaMetafactory'ssemantics in order to be able to perform closure elimination.
toStringin to functions. So rather than printing as<function>, we instead get a rather ugly toString of:$$Lambda$1/914374969@10289886. We would require an extension point inLambdaMetafactory, or ship our ownScalaLambdaMetafactoryto do better here.We cannot yet run the full test suite under this mode, but we are getting close.
Review by @adriaanm @lrytz