Fix problems in Symbol Literal static caching #4095
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Apart from the PR reference raised above, it LGTM. |
|
Commit comment modified, test comments removed. |
|
PLS SYNCH |
|
PLS REBUILD/pr-scala@4a8a5c1e5b37919d77c089bf4e3ccada4b5c3e81 |
|
(kitty-note-to-self: ignore 62108131) |
|
(kitty-note-to-self: ignore 62107930) |
|
@retronym in both commit messages |
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 scala#3149.
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
A classic mistake of discarding a non-trivial qualifier. We actually should have fixed this before merging scala#3149, as it was raised in review, but I suppose we got too caught up in the difficulty of resolving the right overload of `Symbol_apply` that we forgot.
|
Ooops (again.) Fixed. |
|
LGTM |
lrytz
added a commit
that referenced
this pull request
Nov 10, 2014
Fix problems in Symbol Literal static caching
This was referenced Apr 7, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details in the commit messages.
Review by @soc / @lrytz