Skip to content

Commit

Permalink
SI-8933 Disable static Symbol literal cache in traits
Browse files Browse the repository at this point in the history
In Scala 2.8.2, an optimization was added to create a static
cache for Symbol literals (ie, the results of `Symbol.apply("foo"))`.
This saves the map lookup on the second pass through code.

This actually was broken somewhere during the Scala 2.10 series,
after the addition of an overloaded `apply` method to `Symbol`.

The cache synthesis code was made aware of the overload and brought
back to working condition recently, in #3419.

However, this has uncovered a latent bug when the Symbol literals are
defined with traits.

One of the enclosed tests failed with:

	  jvm > t8933b-run.log
	java.lang.IllegalAccessError: tried to access field MotherClass.symbol$1 from class MixinWithSymbol$class
	        at MixinWithSymbol$class.symbolFromTrait(A.scala:3)
	        at MotherClass.symbolFromTrait(Test.scala:1)

This commit simply disables the optimization if we are in a trait.
Alternative fixes might be: a) make the static Symbol cache field
public / b) "mixin" the static symbol cache. Neither of these
seem worth the effort and risk for an already fairly situational
optimization.

Here's how the optimization looks in a class:

	% cat sandbox/test.scala; qscalac sandbox/test.scala && echo ":javap C" | qscala;
	class C {
	  'a; 'b
	}
	Welcome to Scala version 2.11.5-20141106-145558-aa558dce6d (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_20).
	Type in expressions to have them evaluated.
	Type :help for more information.

	scala> :javap C
	  Size 722 bytes
	  MD5 checksum 6bb00189166917254e8d40499ee7c887
	  Compiled from "test.scala"
	public class C

	{
	  public static {};
	    descriptor: ()V
	    flags: ACC_PUBLIC, ACC_STATIC
	    Code:
	      stack=2, locals=0, args_size=0
	         0: getstatic     #16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	         3: ldc           #18                 // String a
	         5: invokevirtual #22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	         8: putstatic     #26                 // Field symbol$1:Lscala/Symbol;
	        11: getstatic     #16                 // Field scala/Symbol$.MODULE$:Lscala/Symbol$;
	        14: ldc           #28                 // String b
	        16: invokevirtual #22                 // Method scala/Symbol$.apply:(Ljava/lang/String;)Lscala/Symbol;
	        19: putstatic     #31                 // Field symbol$2:Lscala/Symbol;
	        22: return

	  public C();
	    descriptor: ()V
	    flags: ACC_PUBLIC
	    Code:
	      stack=1, locals=1, args_size=1
	         0: aload_0
	         1: invokespecial #34                 // Method java/lang/Object."<init>":()V
	         4: getstatic     #26                 // Field symbol$1:Lscala/Symbol;
	         7: pop
	         8: getstatic     #31                 // Field symbol$2:Lscala/Symbol;
	        11: pop
	        12: return
	}

fixup
  • Loading branch information
retronym committed Nov 6, 2014
1 parent cd50464 commit 4a8a5c1
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/transform/CleanUp.scala
Expand Up @@ -520,7 +520,7 @@ abstract class CleanUp extends Statics with Transform with ast.TreeDSL {
* And, finally, be advised - Scala's Symbol literal (scala.Symbol) and the Symbol class of the compiler
* have little in common.
*/
case Apply(fn, (arg @ Literal(Constant(symname: String))) :: Nil) if fn.symbol == Symbol_apply =>
case Apply(fn, (arg @ Literal(Constant(symname: String))) :: Nil) if fn.symbol == Symbol_apply && !currentClass.isTrait =>
def transformApply = {
// add the symbol name to a map if it's not there already
val rhs = gen.mkMethodCall(Symbol_apply, arg :: Nil)
Expand Down
1 change: 1 addition & 0 deletions test/files/run/t8933.check
@@ -0,0 +1 @@
'traitSymbol
6 changes: 6 additions & 0 deletions test/files/run/t8933/A_1.scala
@@ -0,0 +1,6 @@
class MotherClass

trait MixinWithSymbol {
self: MotherClass =>
def symbolFromTrait: Symbol = 'traitSymbol
}
10 changes: 10 additions & 0 deletions test/files/run/t8933/Test_2.scala
@@ -0,0 +1,10 @@
class MotherClass extends MixinWithSymbol {
val classSymbol = 'classSymbol
}

object Test {
def main(args: Array[String]) {
val symbol = (new MotherClass).symbolFromTrait
println(symbol)
}
}
4 changes: 4 additions & 0 deletions test/files/run/t8933b/A.scala
@@ -0,0 +1,4 @@
trait MixinWithSymbol {
self: MotherClass =>
def symbolFromTrait: Any = 'traitSymbol
}
9 changes: 9 additions & 0 deletions test/files/run/t8933b/Test.scala
@@ -0,0 +1,9 @@
class MotherClass extends MixinWithSymbol {
def foo = 'sym1
}

object Test {
def main(args: Array[String]) {
(new MotherClass).symbolFromTrait
}
}

1 comment on commit 4a8a5c1

@scala-jenkins
Copy link

Choose a reason for hiding this comment

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

Job pr-scala failed for 4a8a5c1 Took 127 min. (ping @retronym) (results):


To retry exactly this commit, comment PLS REBUILD/pr-scala@4a8a5c1e5b37919d77c089bf4e3ccada4b5c3e81 on PR 4095.
NOTE: New commits are rebuilt automatically as they appear. A forced rebuild is only necessary for transient failures.

Please sign in to comment.