Skip to content

Disable -Ydelambdafy:method for specialized FunctionN#4367

Merged
adriaanm merged 1 commit intoscala:2.11.xfrom
retronym:topic/indylambda-specialization
Apr 14, 2015
Merged

Disable -Ydelambdafy:method for specialized FunctionN#4367
adriaanm merged 1 commit intoscala:2.11.xfrom
retronym:topic/indylambda-specialization

Conversation

@retronym
Copy link
Member

The Delambdafy phase generates its FunctionN subclasses after
the specialization phase. As such, ((x: Int) => x).apply(42) incurs
boxing.

This commit falls back to the -Ydelambdafy:inline in this case.

This is done by running the specialization type map over the
type of the function, and seeing if anything changes. To make this
work robustly, we first need to ensure that the specialization info
transformer has processed all the function types.

This is not a fundamental limitation; we could in principle generate
the specialized code.

A followup change will use -Ydelambdafy:method as the basis for
invokedymnamic lambdas. As part of that stream of
work, we will synthesize specialization-aware lambdas, and remove
the fallback to -Ydelambdafy:inline.

Review by @gkossakowski

I have updated some tests that indend to test the delambdafy transform
to avoid use of specialized function types.

@scala-jenkins scala-jenkins added this to the 2.11.7 milestone Feb 27, 2015
@gkossakowski
Copy link
Contributor

It looks like this failure is legit:

java.lang.AssertionError: assertion failed: [[syntax trees at end of                delambdafy]] // newSource1.scala
package o {
  package a {
    class C extends Object {
      def hihi(): List = immutable.this.List.apply(scala.this.Predef.wrapIntArray(Array[Int]{1, 2})).map({
  (new <$anon: Function1>(C.this): Function1)
}, immutable.this.List.canBuildFrom()).$asInstanceOf[List]();
      def <init>(): o.a.C = {
        C.super.<init>();
        ()
      }
    };
    @SerialVersionUID(value = 0) final <synthetic> class anonfun$hihi$1 extends scala.runtime.AbstractFunction1$mcII$sp with Serializable {
      final def apply(x$1: Int): Int = anonfun$hihi$1.this.apply$mcII$sp(x$1);
      <specialized> def apply$mcII$sp(x$1: Int): Int = x$1.*(2);
      final <bridge> <artifact> def apply(v1: Object): Object = scala.Int.box(anonfun$hihi$1.this.apply(scala.Int.unbox(v1)));
      def <init>($outer: o.a.C): <$anon: Function1> = {
        anonfun$hihi$1.super.<init>();
        ()
      }
    }
  };
  package a {
    object `package` extends Object {
      def f(): Int = 1;
      def <init>(): o.a.package.type = {
        `package`.super.<init>();
        ()
      }
    }
  }
}


    at Test$.show(t9097.scala:32)

@retronym retronym force-pushed the topic/indylambda-specialization branch 2 times, most recently from d077196 to f4a2528 Compare February 27, 2015 08:29
@retronym
Copy link
Member Author

That was a newly added test in the category of test that "I updated some tests that intend to test the delambdafy transform to avoid use of specialized function types."

I've just changed it to use a non-specialized function so that delambdafy is still used.

@retronym retronym force-pushed the topic/indylambda-specialization branch from f4a2528 to 826f56c Compare March 24, 2015 03:24
@retronym
Copy link
Member Author

Just tweaked another of a test that intends to exercise -Ydelambdafy:method, but was using a (Int => Int) function whose encoding differs after this PR.

@gkossakowski
Copy link
Contributor

/rebuild

I'm not sure why those tests are failing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall. Maybe it is for custom pipelines like ScalaJS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not clear why it's needed, should we remove it? This kind of code is hard to remove later on because everybody fears some hard to predict breakage.

The Delambdafy phase generates its `FunctionN` subclasses after
the specialization phase. As such, `((x: Int) => x).apply(42)` incurs
boxing.

This commit falls back to the `-Ydelambdafy:inline` in this case.

This is done by running the specialization type map over the
type of the function, and seeing if anything changes. To make this
work robustly, we first need to ensure that the specialization info
transformer has processed all the function types.

This is not a fundamental limitation; we could in principle generate
the specialized code.

A followup change will use `-Ydelambdafy:method` as the basis for
invokedymnamic lambdas. As part of that stream of
work, we will synthesize specialization-aware lambdas, and remove
the fallback to `-Ydelambdafy:inline`.

I have updated some tests that intend to test the delambdafy transform
to avoid use of specialized function types.
@retronym retronym force-pushed the topic/indylambda-specialization branch from 8db3ef9 to ea61b3f Compare April 10, 2015 02:04
@retronym
Copy link
Member Author

@gkossakowski By removing that condition, I found that a test case using -Ystop-after uncurry failed. That must have been what prompted by to add this code in the first place. I have reinstated it with a supporting comment.

@retronym
Copy link
Member Author

@adriaanm / @lrytz could I ask for a review of this one?

@adriaanm
Copy link
Contributor

LGTM -- Merging to unlock next steps. There's a small typo btw. Left as exercise for the submitter :)

adriaanm added a commit that referenced this pull request Apr 14, 2015
Disable -Ydelambdafy:method for specialized FunctionN
@adriaanm adriaanm merged commit 029cce7 into scala:2.11.x Apr 14, 2015
@adriaanm
Copy link
Contributor

I was just about to hit the merge button, @retronym -- meant to do sooner, sorry.

@adriaanm
Copy link
Contributor

PS: canUseDelam_b_dafyMethod

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants