String interpolation optimization miniphase #3961

Wojtechnology · 2018-02-04T17:45:40Z

Addresses #3308. s and raw string interpolators use string concatenation instead of StringContext. Seeing a 4x increase in performance on a very simple initial benchmark:

With optimization:

[info] Benchmark                  Mode  Cnt      Score       Error   Units
[info] Test.testRawInterpolator  thrpt    3  42419.376 ±  8525.679  ops/ms
[info] Test.testSInterpolator    thrpt    3  41732.471 ±  7622.324  ops/ms
[info] Test.testStrConcat        thrpt    3  40346.563 ± 14475.086  ops/ms

Without:

[info] Benchmark                  Mode  Cnt      Score      Error   Units
[info] Test.testRawInterpolator  thrpt    3   9304.561 ±  996.445  ops/ms
[info] Test.testSInterpolator    thrpt    3   8325.524 ±  677.976  ops/ms
[info] Test.testStrConcat        thrpt    3  40857.625 ± 3487.954  ops/ms

Performance for string interpolators is on par with string concatenation.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

allanrenucci · 2018-02-05T16:46:22Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+          else if (id == nme.s) {
+            try {
+              val escapedStrs = strs.mapConserve { str =>
+                val strValue = str.asInstanceOf[Literal].const.stringValue


I don't think you can assume that str is always a constant literal. One could write:

val foo = "Hello" StringContext(foo).s()

allanrenucci · 2018-02-05T16:49:52Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+
+  override def transformApply(tree: Apply)(implicit ctx: Context): Tree = {
+    tree match {
+      case StringContextIntrinsic(strs: List[Tree], elems: List[Tree]) =>


I would do something similar to https://github.com/scala/scala/blob/c2a5883891a68180b143eb462c8b0cebc8d3b021/src/library/scala/StringContext.scala#L119. It is very hard to understand what the code below does

smarter · 2018-02-05T18:50:17Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+  private object StringContextIntrinsic {
+    def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Tree], List[Tree])] = {
+      tree match {
+        case Apply(Select(Apply(Select(Ident(nme.StringContext), nme.apply),


We cannot rely on names alone to enable this optimization. There might be some other class called StringContext somewhere. Instead you should check the .symbol of the tree to make sure it's scala.StringContext. You can add something like this to https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/core/Definitions.scala:

lazy val StringContextType: TypeRef = ctx.requiredClassRef("scala.StringContext") def StringContextClass(implicit ctx: Context) = StringContextType.symbol.asClass

and then defn.StringContextClass will be the symbol you want to check against.

Wojtechnology · 2018-02-11T19:58:24Z

Do we usually write unit tests for mini phases? In particular, I'd like to check if the correct tree is generated after passing it to the phase. Is there any example of this kind of test?

smarter · 2018-02-11T20:35:39Z

We rarely unit test mini phases (I think there's some in https://github.com/lampepfl/dotty/tree/master/compiler/test/dotty/tools/dotc/transform), we usually do end-to-end testing instead. Here I would suggest adding some tests that check that the same bytecode is generated when using the interpolator or when writing the equivalent code by hand, see https://github.com/lampepfl/dotty/blob/master/compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala for example.

smarter · 2018-02-12T09:33:59Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+        case Apply(Select(Apply(Select(ident, nme.apply), List(SeqLiteral(strs, _))), fn),
+            List(SeqLiteral(elems, _))) =>
+          val clsSym = ident.symbol.companionClass
+          if (clsSym.eq(defn.StringContextClass) && strs.forall(_.isInstanceOf[Literal])


You could check if ident.symbol is equal to defn.StringContextModuleClass instead after adding StringContextModuleClass to Definitions.scala

odersky · 2018-03-19T17:14:59Z

What is the status of the PR? Given the performance improvements it would be great to get this in.

Wojtechnology · 2018-03-20T22:12:16Z

Just waiting on code review from my end.

allanrenucci · 2018-03-21T15:59:10Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+
+  private object StringContextIntrinsic {
+    def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Tree], List[Tree])] = {
+      tree match {


I would first check if tree.symbol is StringContext.s or StringContext.raw for performance

allanrenucci · 2018-03-21T17:30:24Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+
+/**
+  * Created by wojtekswiderski on 2018-01-24.
+  */


You can remove the author attribution. Also add a comment explaining what this phase does

allanrenucci · 2018-03-21T17:30:28Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+
+  override def phaseName: String = "stringInterpolatorOpt"
+
+  private object StringContextIntrinsic {


Add a comment explaining what this extractor does

allanrenucci · 2018-03-21T17:40:03Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+  private object StringContextIntrinsic {
+    def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Tree], List[Tree])] = {
+      tree match {
+        case Apply(Select(Apply(Select(ident, nme.apply), List(SeqLiteral(strs, _))), fn),


Comparing names is not reliable, prefer symbols (for StringContext.apply)

allanrenucci · 2018-03-21T17:41:40Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+            if (fn == nme.raw_) Some(strs, elems)
+            else if (fn == nme.s) {
+              try {
+                val escapedStrs = strs.mapConserve { str =>


mapConserve is not useful here, use map

allanrenucci · 2018-03-21T17:57:42Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+  private object StringContextIntrinsic {
+    def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Tree], List[Tree])] = {
+      tree match {
+        case Apply(Select(Apply(Select(ident, nme.apply), List(SeqLiteral(strs, _))), fn),


You could define an extractor for a sequence of literals to avoid the multiple casts:

/** Match a list of constant literals */ object Literals { def unapply(tree: SeqLiteral): Option[List[Literal]] = ... }

And then change the return type of the StringContextIntrinsic extractor:

object StringContextIntrinsic { def unapply(tree: Apply)(implicit ctx: Context): Option[(List[Literal], List[Tree])] = ... }

allanrenucci · 2018-03-21T18:04:52Z

compiler/test/dotty/tools/backend/jvm/StringInterpolatorOptTest.scala

+                   |  def meth1: String = raw"$one plus $two$three\n"
+                   |  def meth2: String = "" + one + " plus " + two + three + "\\n"
+                   |}
+                 """.stripMargin


Use normal indentation:

""" |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

allanrenucci · 2018-03-21T18:05:23Z

compiler/test/dotty/tools/backend/jvm/StringInterpolatorOptTest.scala

+                   |  def meth1: String = s"$one plus $two$three\n"
+                   |  def meth2: String = "" + one + " plus " + two + three + "\n"
+                   |}
+                 """.stripMargin


Use normal indentation:

allanrenucci · 2018-03-26T11:48:42Z

tests/run/interpolation-opt.scala

+  // Make sure that StringContext still works with idents
+  val foo = "Hello"
+  val bar = "World"
+  println(StringContext(foo, bar).s(" "))


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

Added the test and looks like it handles it correctly.

Sorry, my example was erroneous. What I had is mind was:

println(myStringContext("Foo", "Bar").s(" ")) // this shouldn't be optimised away

This one too, should not be optimised:

println({ println("Side effect n2!"); StringContext }.apply("Titi", "Toto").s(" ")) // this shouldn't be optimised away

Added tests and a fix.

allanrenucci · 2018-03-26T11:52:42Z

compiler/src/dotty/tools/dotc/core/Definitions.scala

+    def StringContextRaw(implicit ctx: Context) = StringContextRawR.symbol
+  def StringContextModule(implicit ctx: Context) = StringContextClass.companionModule
+    lazy val StringContextModule_applyR = StringContextModule.requiredMethodRef(nme.apply)
+    def StringContextModule_apply(implicit ctx: Context) = StringContextModule_applyR.symbol


@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 in prepareForUnit?

allanrenucci · 2018-04-09T08:38:14Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+    def unapply(tree: Ident)(implicit ctx: Context): Option[Ident] = {
+      if (tree.symbol.eq(defn.StringContextModule)) Some(tree) else None
+    }
+  }


Inline this extractor into StringContextApply

allanrenucci · 2018-04-09T08:45:29Z

compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

+    }
+  }
+
+  private object StringContextApply {


I see that this extractor is just a test and does not return any result (but the tree itself). You can return a Boolean instead:

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) } }

allanrenucci · 2018-04-20T15:37:52Z

test performance please

dottybot · 2018-04-20T15:38:14Z

performance test scheduled: 2 job(s) in queue, 1 running.

dottybot · 2018-04-20T18:34:32Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3961/ to see the changes.

Benchmarks is based on merging with master (f2bb231)

OlivierBlanvillain · 2018-05-01T10:14:49Z

@Wojtechnology thanks!

…-quotes FIx source of performance regression in #3961

dottybot reviewed Feb 4, 2018

View reviewed changes

allanrenucci suggested changes Feb 5, 2018

View reviewed changes

smarter reviewed Feb 5, 2018

View reviewed changes

smarter reviewed Feb 12, 2018

View reviewed changes

OlivierBlanvillain assigned Wojtechnology Mar 6, 2018

Wojtechnology force-pushed the ws-optimize-string-interpolation branch from ecab729 to 474ff95 Compare March 12, 2018 22:51

smarter assigned allanrenucci and unassigned Wojtechnology Mar 20, 2018

allanrenucci suggested changes Mar 21, 2018

View reviewed changes

Wojtechnology added 6 commits March 25, 2018 11:06

String interpolation optimization miniphase

39c3bc8

Addressed comments

f7c2208

Use companion class

f19e12b

fix test

5fed68f

StringContextClass -> Module

0b1e4ac

Added bytecode tests and fixed module defn

11e6975

Wojtechnology force-pushed the ws-optimize-string-interpolation branch from ad3cf75 to 11e6975 Compare March 25, 2018 15:08

Addressed comments

2da90a0

allanrenucci reviewed Mar 26, 2018

View reviewed changes

Wojtechnology added 3 commits April 7, 2018 21:35

Merge branch 'master' into ws-optimize-string-interpolation

053d509

Added test

7626e05

New tests and fix

ac7ab0a

allanrenucci reviewed Apr 9, 2018

View reviewed changes

Simplify extractors

449aff2

allanrenucci approved these changes Apr 20, 2018

View reviewed changes

allanrenucci merged commit e87a5d1 into scala:master May 1, 2018

nicolasstucki added a commit that referenced this pull request May 11, 2018

Merge pull request #4480 from dotty-staging/performance-regression-on…

97f4155

…-quotes FIx source of performance regression in #3961

allanrenucci mentioned this pull request May 13, 2018

Optimize the raw"" and s"" string interpolators #3308

Closed


		override def phaseName: String = "stringInterpolatorOpt"

		private object StringContextIntrinsic {

String interpolation optimization miniphase #3961

String interpolation optimization miniphase #3961

Uh oh!

Conversation

Wojtechnology commented Feb 4, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wojtechnology commented Feb 11, 2018

Uh oh!

smarter commented Feb 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky commented Mar 19, 2018

Uh oh!

Wojtechnology commented Mar 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Apr 20, 2018

Uh oh!

dottybot commented Apr 20, 2018

Uh oh!

dottybot commented Apr 20, 2018

Uh oh!

OlivierBlanvillain commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

allanrenucci Feb 5, 2018 •

edited

Loading

smarter Feb 5, 2018 •

edited

Loading