Skip to content
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.

Sign up for GitHub

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

Jump to bottom

A handful of tests for closures under reification #79

Merged
merged 1 commit into from Dec 27, 2011
Merged

A handful of tests for closures under reification #79

merged 1 commit into from Dec 27, 2011

Conversation

xeno-by
Copy link
Member

@xeno-by xeno-by commented Dec 26, 2011

No description provided.

@paulp paulp merged commit 8f2d318 into scala:master Dec 27, 2011
retronym referenced this pull request in retronym/scala Dec 2, 2012
@retronym
SI-6754 Position ClassTag based unapply
414f25e 
Range Positions didn't take kindly to it.

======= Position error
Unpositioned tree scala#483
   unpositioned [L  -1 P#  484] scala#483    [NoPosition]    UnApply    // UnApply
      enclosing [L   7        ] scala#484    [261:291]       CaseDef    // ru.TypeRef(_, _, args) => args;
        sibling [L  -1 P#  484] scala#483    [NoPosition]    UnApply    // UnApply
        sibling [L   7 P#  484] #79     [287:291]       Ident      // args;
@retronym retronym mentioned this pull request Dec 2, 2012
Closed
retronym referenced this pull request in retronym/scala May 4, 2016
@retronym
Make all lambda impl methods static
6b466ae 
Motivation:

  - Avoid introducing public virtual methods. (javac uses private methods,
    but we prefer to make the public to support important AOT inlining
    use cases)
     - No more need for unsightly expanded names in lambda stack traces!
     - CHA in on HotSpot is great at devirtualizing, but that doesn't mean
       we *should* emit non-virtual methods as virtual so pervasively.

```
// Entering paste mode (ctrl-D to finish)

package com.acme.wizzle.wozzle; class C { def foo = () => ??? }

// Exiting paste mode, now interpreting.

scala> new com.acme.wizzle.wozzle.C().foo
res0: () => Nothing = com.acme.wizzle.wozzle.C$$Lambda$1986/43856716@100f9bbe

scala> new com.acme.wizzle.wozzle.C().foo.apply()
scala.NotImplementedError: an implementation is missing
  at scala.Predef$.$qmark$qmark$qmark(Predef.scala:284)
  at com.acme.wizzle.wozzle.C.$anonfun$1(<pastie>:1)
  ... 30 elided

scala> :paste -raw
// Entering paste mode (ctrl-D to finish)

package p1; class StaticAllTheThings { def foo = () => ""; def bar = () => foo; def baz = () => this }

// Exiting paste mode, now interpreting.

scala> :javap -private -c p1.StaticAllTheThings
Compiled from "<pastie>"
public class p1.StaticAllTheThings {
  public scala.Function0<java.lang.String> foo();
    Code:
       0: invokedynamic #38,  0             // InvokeDynamic #0:apply:()Lscala/Function0;
       5: areturn

  public scala.Function0<scala.Function0<java.lang.String>> bar();
    Code:
       0: aload_0
       1: invokedynamic #49,  0             // InvokeDynamic #1:apply:(Lp1/StaticAllTheThings;)Lscala/Function0;
       6: areturn

  public scala.Function0<p1.StaticAllTheThings> baz();
    Code:
       0: aload_0
       1: invokedynamic #58,  0             // InvokeDynamic #2:apply:(Lp1/StaticAllTheThings;)Lscala/Function0;
       6: areturn

  public static final java.lang.String $anonfun$1();
    Code:
       0: ldc           #60                 // String
       2: areturn

  public static final scala.Function0 $anonfun$2(p1.StaticAllTheThings);
    Code:
       0: aload_0
       1: invokevirtual #63                 // Method foo:()Lscala/Function0;
       4: areturn

  public static final p1.StaticAllTheThings $anonfun$3(p1.StaticAllTheThings);
    Code:
       0: aload_0
       1: areturn

  public p1.StaticAllTheThings();
    Code:
       0: aload_0
       1: invokespecial #67                 // Method java/lang/Object."<init>":()V
       4: return

  private static java.lang.Object $deserializeLambda$(java.lang.invoke.SerializedLambda);
    Code:
       0: aload_0
       1: invokedynamic #79,  0             // InvokeDynamic #3:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
       6: areturn
}
```
retronym referenced this pull request in retronym/scala May 5, 2016
@retronym
Make all lambda impl methods static
3ca962c 
Motivation:

  - Avoid introducing public virtual methods. (javac uses private methods,
    but we prefer to make the public to support important AOT inlining
    use cases)
     - No more need for unsightly expanded names in lambda stack traces!
     - CHA in on HotSpot is great at devirtualizing, but that doesn't mean
       we *should* emit non-virtual methods as virtual so pervasively.

```
// Entering paste mode (ctrl-D to finish)

package com.acme.wizzle.wozzle; class C { def foo = () => ??? }

// Exiting paste mode, now interpreting.

scala> new com.acme.wizzle.wozzle.C().foo
res0: () => Nothing = com.acme.wizzle.wozzle.C$$Lambda$1986/43856716@100f9bbe

scala> new com.acme.wizzle.wozzle.C().foo.apply()
scala.NotImplementedError: an implementation is missing
  at scala.Predef$.$qmark$qmark$qmark(Predef.scala:284)
  at com.acme.wizzle.wozzle.C.$anonfun$1(<pastie>:1)
  ... 30 elided

scala> :paste -raw
// Entering paste mode (ctrl-D to finish)

package p1; class StaticAllTheThings { def foo = () => ""; def bar = () => foo; def baz = () => this }

// Exiting paste mode, now interpreting.

scala> :javap -private -c p1.StaticAllTheThings
Compiled from "<pastie>"
public class p1.StaticAllTheThings {
  public scala.Function0<java.lang.String> foo();
    Code:
       0: invokedynamic #38,  0             // InvokeDynamic #0:apply:()Lscala/Function0;
       5: areturn

  public scala.Function0<scala.Function0<java.lang.String>> bar();
    Code:
       0: aload_0
       1: invokedynamic #49,  0             // InvokeDynamic #1:apply:(Lp1/StaticAllTheThings;)Lscala/Function0;
       6: areturn

  public scala.Function0<p1.StaticAllTheThings> baz();
    Code:
       0: aload_0
       1: invokedynamic #58,  0             // InvokeDynamic #2:apply:(Lp1/StaticAllTheThings;)Lscala/Function0;
       6: areturn

  public static final java.lang.String $anonfun$1();
    Code:
       0: ldc           #60                 // String
       2: areturn

  public static final scala.Function0 $anonfun$2(p1.StaticAllTheThings);
    Code:
       0: aload_0
       1: invokevirtual #63                 // Method foo:()Lscala/Function0;
       4: areturn

  public static final p1.StaticAllTheThings $anonfun$3(p1.StaticAllTheThings);
    Code:
       0: aload_0
       1: areturn

  public p1.StaticAllTheThings();
    Code:
       0: aload_0
       1: invokespecial #67                 // Method java/lang/Object."<init>":()V
       4: return

  private static java.lang.Object $deserializeLambda$(java.lang.invoke.SerializedLambda);
    Code:
       0: aload_0
       1: invokedynamic #79,  0             // InvokeDynamic #3:lambdaDeserialize:(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;
       6: areturn
}
```
szeiger pushed a commit to szeiger/scala that referenced this pull request Mar 20, 2018
@odersky
Fix scala#79: Add test
049cb47 
scala#79 is fixed by scala#2435 in the dotty repo. This PR contains the test that failed.
szeiger pushed a commit to szeiger/scala that referenced this pull request Mar 20, 2018
@julienrf
Merge pull request scala#84 from scala/fix-#79
b8983b3 
Fix scala#79: Add test
olafurpg added a commit to olafurpg/scala that referenced this pull request Jul 17, 2018
@olafurpg
Revive productElementName method to print field names
b999d4e 
This commit uncomments the disabled `productElementName` method.

The benefit of this change is that it becomes possible to pretty-print
`Product` instances with field, for example
```scala
case class User(name: String, age: Int)

def pretty(p: Product): String =
  p.productElementNames.zip(p.productIterator)
   .map { case (name, value) => s"$name=$value" }
   .mkString(p.productPrefix + "(", ", ", ")")

toPrettyProduct(User("Susan", 42))
// res0: String = User(name=Susan, age=42)
```

The downside of this change is that it produces more bytecode for each
case-class definition.  I am not versed enough in reading javap output
to determine the exact bytecode size for a given method. I tried the
following:
```scala
> case class A(a: Int, b: Int, c: Int)
> :javap -c A
  public java.lang.String productElementName(int);
    Code:
       0: iload_1
       1: istore_2
       2: iload_2
       3: tableswitch   { // 0 to 2
                     0: 28
                     1: 33
                     2: 38
               default: 43
          }
      28: ldc           scala#78                 // String a
      30: goto          58
      33: ldc           scala#79                 // String b
      35: goto          58
      38: ldc           scala#80                 // String c
      40: goto          58
      43: new           scala#67                 // class java/lang/IndexOutOfBoundsException
      46: dup
      47: iload_1
      48: invokestatic  scala#65                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      51: invokevirtual scala#70                 // Method java/lang/Object.toString:()Ljava/lang/String;
      54: invokespecial scala#73                 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V
      57: athrow
      58: areturn
```
olafurpg added a commit to olafurpg/scala that referenced this pull request Jul 17, 2018
@olafurpg
Revive productElementName method to print field names
8430f69 
This commit uncomments the disabled `productElementName` method.

The benefit of this change is that it becomes possible to pretty-print
`Product` instances with field, for example
```scala
case class User(name: String, age: Int)

def pretty(p: Product): String =
  p.productElementNames.zip(p.productIterator)
   .map { case (name, value) => s"$name=$value" }
   .mkString(p.productPrefix + "(", ", ", ")")

toPrettyProduct(User("Susan", 42))
// res0: String = User(name=Susan, age=42)
```

The downside of this change is that it produces more bytecode for each
case-class definition.  I am not versed enough in reading javap output
to determine the exact bytecode size for a given method. I tried the
following:
```scala
> case class A(a: Int, b: Int, c: Int)
> :javap -c A
  public java.lang.String productElementName(int);
    Code:
       0: iload_1
       1: istore_2
       2: iload_2
       3: tableswitch   { // 0 to 2
                     0: 28
                     1: 33
                     2: 38
               default: 43
          }
      28: ldc           scala#78                 // String a
      30: goto          58
      33: ldc           scala#79                 // String b
      35: goto          58
      38: ldc           scala#80                 // String c
      40: goto          58
      43: new           scala#67                 // class java/lang/IndexOutOfBoundsException
      46: dup
      47: iload_1
      48: invokestatic  scala#65                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      51: invokevirtual scala#70                 // Method java/lang/Object.toString:()Ljava/lang/String;
      54: invokespecial scala#73                 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V
      57: athrow
      58: areturn
```
olafurpg added a commit to olafurpg/scala that referenced this pull request Jul 18, 2018
@olafurpg
Revive productElementName method to print field names
778cf37 
This commit uncomments the disabled `productElementName` method.

The benefit of this change is that it becomes possible to pretty-print
`Product` instances with field, for example
```scala
case class User(name: String, age: Int)

def pretty(p: Product): String =
  p.productElementNames.zip(p.productIterator)
   .map { case (name, value) => s"$name=$value" }
   .mkString(p.productPrefix + "(", ", ", ")")

toPrettyProduct(User("Susan", 42))
// res0: String = User(name=Susan, age=42)
```

The downside of this change is that it produces more bytecode for each
case-class definition. Running `:javacp -c` for a case class with three
fields yields the following results
```scala
> case class A(a: Int, b: Int, c: Int)
> :javap -c A
  public java.lang.String productElementName(int);
    Code:
       0: iload_1
       1: istore_2
       2: iload_2
       3: tableswitch   { // 0 to 2
                     0: 28
                     1: 33
                     2: 38
               default: 43
          }
      28: ldc           scala#78                 // String a
      30: goto          58
      33: ldc           scala#79                 // String b
      35: goto          58
      38: ldc           scala#80                 // String c
      40: goto          58
      43: new           scala#67                 // class java/lang/IndexOutOfBoundsException
      46: dup
      47: iload_1
      48: invokestatic  scala#65                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      51: invokevirtual scala#70                 // Method java/lang/Object.toString:()Ljava/lang/String;
      54: invokespecial scala#73                 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V
      57: athrow
      58: areturn
```

Thanks to Adriaan's help, the estimated cost per `productElementName`
appears to be fixed 56 bytes and then 10 bytes for each field with
the following breakdown:

* 3 bytes for the
  [string info](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.3)
  (the actual characters are already in the constant pool)
* 4 bytes for the tableswitch entry
* 2 bytes for the ldc to load the string
* 1 byte for areturn

In my opinion, the bytecode cost is acceptably low thanks to the fact
that field name literals are already available in the constant pool.
olafurpg added a commit to olafurpg/scala that referenced this pull request Jul 18, 2018
@olafurpg
Revive productElementName to extract case class field names
55d5223 
This commit adds two methods to the `scala.Product` trait:
```scala
trait Product {
  /** Returns the field name of element at index n */
  def productElementName(n: Int): String
  /** Returns field names of this product. Must have same length as productIterator */
  def productElementNames: Iterator[String]
}
```

Both methods have a default implementation which returns the empty
string for all field names.

This commit then changes the code-generation for case classes to
synthesize a `productElementName` method with actual class field names.

The benefit of this change is that it becomes possible to pretty-print
case classes with field names, for example
```scala
case class User(name: String, age: Int)

def toPrettyString(p: Product): String =
  p.productElementNames.zip(p.productIterator)
   .map { case (name, value) => s"$name=$value" }
   .mkString(p.productPrefix + "(", ", ", ")")

toPrettyString(User("Susan", 42))
// res0: String = User(name=Susan, age=42)
```

The downside of this change is that it produces more bytecode for each
case-class definition. Running `:javacp -c` for a case class with three
fields yields the following results
```scala
> case class A(a: Int, b: Int, c: Int)
> :javap -c A
  public java.lang.String productElementName(int);
    Code:
       0: iload_1
       1: istore_2
       2: iload_2
       3: tableswitch   { // 0 to 2
                     0: 28
                     1: 33
                     2: 38
               default: 43
          }
      28: ldc           scala#78                 // String a
      30: goto          58
      33: ldc           scala#79                 // String b
      35: goto          58
      38: ldc           scala#80                 // String c
      40: goto          58
      43: new           scala#67                 // class java/lang/IndexOutOfBoundsException
      46: dup
      47: iload_1
      48: invokestatic  scala#65                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      51: invokevirtual scala#70                 // Method java/lang/Object.toString:()Ljava/lang/String;
      54: invokespecial scala#73                 // Method java/lang/IndexOutOfBoundsException."<init>":(Ljava/lang/String;)V
      57: athrow
      58: areturn
```

Thanks to Adriaan's help, the estimated cost per `productElementName`
appears to be fixed 56 bytes and then 10 bytes for each field with
the following breakdown:

* 3 bytes for the
  [string info](https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.3)
  (the actual characters are already in the constant pool)
* 4 bytes for the tableswitch entry
* 2 bytes for the ldc to load the string
* 1 byte for areturn

In my opinion, the bytecode cost is acceptably low thanks to the fact
that field name literals are already available in the constant pool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@xeno-by @paulp