-
Notifications
You must be signed in to change notification settings - Fork 1.1k
String interpolation optimization miniphase #3961
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.
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
Changes from 10 commits
39c3bc8
f7c2208
f19e12b
5fed68f
0b1e4ac
11e6975
2da90a0
053d509
7626e05
ac7ab0a
449aff2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
package dotty.tools.dotc.transform.localopt | ||
|
||
import dotty.tools.dotc.ast.Trees._ | ||
import dotty.tools.dotc.ast.tpd | ||
import dotty.tools.dotc.core.Constants.Constant | ||
import dotty.tools.dotc.core.Contexts.Context | ||
import dotty.tools.dotc.core.StdNames._ | ||
import dotty.tools.dotc.core.Symbols._ | ||
import dotty.tools.dotc.transform.MegaPhase.MiniPhase | ||
|
||
/** | ||
* MiniPhase to transform s and raw string interpolators from using StringContext to string | ||
* concatenation. Since string concatenation uses the Java String builder, we get a performance | ||
* improvement in terms of these two interpolators. | ||
* | ||
* More info here: | ||
* https://medium.com/@dkomanov/scala-string-interpolation-performance-21dc85e83afd | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove the author attribution. Also add a comment explaining what this phase does |
||
class StringInterpolatorOpt extends MiniPhase { | ||
import tpd._ | ||
|
||
override def phaseName: String = "stringInterpolatorOpt" | ||
|
||
/** Matches a list of constant literals */ | ||
private object Literals { | ||
def unapply(tree: SeqLiteral)(implicit ctx: Context): Option[List[Literal]] = { | ||
tree.elems match { | ||
case literals if literals.forall(_.isInstanceOf[Literal]) => | ||
Some(literals.map(_.asInstanceOf[Literal])) | ||
case _ => None | ||
} | ||
} | ||
} | ||
|
||
private object StringContextIdent { | ||
def unapply(tree: Ident)(implicit ctx: Context): Option[Ident] = { | ||
if (tree.symbol.eq(defn.StringContextModule)) Some(tree) else None | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inline this extractor into |
||
|
||
private object StringContextApply { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that this extractor is just a test and does not return any result (but the tree itself). You can return a private object StringContextApply {
def unapply(tree: Select)(implicit ctx: Context): Boolean =
tree.symbol.eq(defn.StringContextModule_apply) && {
val qualifier = tree.qualifier
qualifier.isInstanceOf[Ident] && qualifier.symbol.eq(efn.StringContextModule)
}
} |
||
def unapply(tree: Select)(implicit ctx: Context): Option[Select] = { | ||
tree match { | ||
case Select(StringContextIdent(_), _) | ||
if tree.symbol.eq(defn.StringContextModule_apply) => Some(tree) | ||
case _ => None | ||
} | ||
} | ||
} | ||
|
||
/** Matches an s or raw string interpolator */ | ||
private object SOrRawInterpolator { | ||
def unapply(tree: Tree)(implicit ctx: Context): Option[(List[Literal], List[Tree])] = { | ||
if (tree.symbol.eq(defn.StringContextRaw) || tree.symbol.eq(defn.StringContextS)) { | ||
tree match { | ||
case Apply(Select(Apply(StringContextApply(_), List(Literals(strs))), _), | ||
List(SeqLiteral(elems, _))) if elems.length == strs.length - 1 => | ||
Some(strs, elems) | ||
case _ => None | ||
} | ||
} else None | ||
} | ||
} | ||
|
||
/** | ||
* Match trees that resemble s and raw string interpolations. In the case of the s | ||
* interpolator, escapes the string constants. Exposes the string constants as well as | ||
* the variable references. | ||
*/ | ||
private object StringContextIntrinsic { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment explaining what this extractor does |
||
def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Literal], List[Tree])] = { | ||
tree match { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would first check if |
||
case SOrRawInterpolator(strs, elems) => | ||
if (tree.symbol == defn.StringContextRaw) Some(strs, elems) | ||
else { // tree.symbol == defn.StringContextS | ||
try { | ||
val escapedStrs = strs.map { str => | ||
val escapedValue = StringContext.processEscapes(str.const.stringValue) | ||
cpy.Literal(str)(Constant(escapedValue)) | ||
} | ||
Some(escapedStrs, elems) | ||
} catch { | ||
case _: StringContext.InvalidEscapeException => None | ||
} | ||
} | ||
case _ => None | ||
} | ||
} | ||
} | ||
|
||
override def transformApply(tree: Apply)(implicit ctx: Context): Tree = { | ||
tree match { | ||
case StringContextIntrinsic(strs: List[Literal], elems: List[Tree]) => | ||
val stri = strs.iterator | ||
val elemi = elems.iterator | ||
var result: Tree = stri.next | ||
def concat(tree: Tree): Unit = { | ||
result = result.select(defn.String_+).appliedTo(tree) | ||
} | ||
while (elemi.hasNext) { | ||
concat(elemi.next) | ||
val str = stri.next | ||
if (!str.const.stringValue.isEmpty) concat(str) | ||
} | ||
result | ||
case _ => tree | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
package dotty.tools.backend.jvm | ||
|
||
import org.junit.Assert._ | ||
import org.junit.Test | ||
|
||
class StringInterpolatorOptTest extends DottyBytecodeTest { | ||
import ASMConverters._ | ||
|
||
@Test def testRawInterpolator = { | ||
val source = | ||
""" | ||
|class Foo { | ||
| val one = 1 | ||
| val two = "two" | ||
| val three = 3.0 | ||
| | ||
| def meth1: String = raw"$one plus $two$three\n" | ||
| def meth2: String = "" + one + " plus " + two + three + "\\n" | ||
|} | ||
""".stripMargin | ||
|
||
checkBCode(source) { dir => | ||
val clsIn = dir.lookupName("Foo.class", directory = false).input | ||
val clsNode = loadClassNode(clsIn) | ||
val meth1 = getMethod(clsNode, "meth1") | ||
val meth2 = getMethod(clsNode, "meth2") | ||
|
||
val instructions1 = instructionsFromMethod(meth1) | ||
val instructions2 = instructionsFromMethod(meth2) | ||
|
||
assert(instructions1 == instructions2, | ||
"the `` string interpolator incorrectly converts to string concatenation\n" + | ||
diffInstructions(instructions1, instructions2)) | ||
} | ||
} | ||
|
||
@Test def testSInterpolator = { | ||
val source = | ||
""" | ||
|class Foo { | ||
| val one = 1 | ||
| val two = "two" | ||
| val three = 3.0 | ||
| | ||
| def meth1: String = s"$one plus $two$three\n" | ||
| def meth2: String = "" + one + " plus " + two + three + "\n" | ||
|} | ||
""".stripMargin | ||
|
||
checkBCode(source) { dir => | ||
val clsIn = dir.lookupName("Foo.class", directory = false).input | ||
val clsNode = loadClassNode(clsIn) | ||
val meth1 = getMethod(clsNode, "meth1") | ||
val meth2 = getMethod(clsNode, "meth2") | ||
|
||
val instructions1 = instructionsFromMethod(meth1) | ||
val instructions2 = instructionsFromMethod(meth2) | ||
|
||
assert(instructions1 == instructions2, | ||
"the `s` string interpolator incorrectly converts to string concatenation\n" + | ||
diffInstructions(instructions1, instructions2)) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
1 plus two\nis 3.0 | ||
1 plus two | ||
is 3.0 | ||
a1two3.0b | ||
a1two3.0b | ||
|
||
|
||
Hello World | ||
Side effect! | ||
Foo Bar | ||
Side effect n2! | ||
Titi Toto |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
object Test extends App { | ||
|
||
val one = 1 | ||
val two = "two" | ||
val three = 3.0 | ||
|
||
// Test escaping | ||
println(raw"$one plus $two\nis $three") | ||
println(s"$one plus $two\nis $three") | ||
|
||
// Test empty strings between elements | ||
println(raw"a$one$two${three}b") | ||
println(s"a$one$two${three}b") | ||
|
||
// Test empty string interpolators | ||
println(raw"") | ||
println(s"") | ||
|
||
// Make sure that StringContext still works with idents | ||
val foo = "Hello" | ||
val bar = "World" | ||
println(StringContext(foo, bar).s(" ")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for: def myStringContext= { println("Side effect!"); StringContext }
println(myStringContext(foo, bar).s(" ")) // this shouldn't be optimised away I am not sure it is handled by your implementation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the test and looks like it handles it correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, my example was erroneous. What I had is mind was: println(myStringContext("Foo", "Bar").s(" ")) // this shouldn't be optimised away
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one too, should not be optimised: println({ println("Side effect n2!"); StringContext }.apply("Titi", "Toto").s(" ")) // this shouldn't be optimised away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added tests and a fix. |
||
|
||
def myStringContext= { println("Side effect!"); StringContext } | ||
println(myStringContext("Foo", "Bar").s(" ")) // this shouldn't be optimised away | ||
|
||
// this shouldn't be optimised away | ||
println({ println("Side effect n2!"); StringContext }.apply("Titi", "Toto").s(" ")) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarter Can you comment on this? Should we expose only the ones used in the optimisation (i.e.
StringContextRaw
,StringContextS
,StringContextModule_apply
)? Should these values be inlined in the mini phase and be initialised inprepareForUnit
?