Skip to content

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.

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

Merged
merged 11 commits into from
May 1, 2018
Merged

String interpolation optimization miniphase #3961

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
39c3bc8
String interpolation optimization miniphase
Wojtechnology Feb 3, 2018
f7c2208
Addressed comments
Wojtechnology Feb 11, 2018
f19e12b
Use companion class
Wojtechnology Feb 11, 2018
5fed68f
fix test
Wojtechnology Feb 12, 2018
0b1e4ac
StringContextClass -> Module
Wojtechnology Mar 12, 2018
11e6975
Added bytecode tests and fixed module defn
Wojtechnology Mar 12, 2018
2da90a0
Addressed comments
Wojtechnology Mar 25, 2018
053d509
Merge branch 'master' into ws-optimize-string-interpolation
Wojtechnology Apr 8, 2018
7626e05
Added test
Wojtechnology Apr 8, 2018
ac7ab0a
New tests and fix
Wojtechnology Apr 8, 2018
449aff2
Simplify extractors
Wojtechnology Apr 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,29 @@ class StringInterpolatorOpt extends MiniPhase {
}
}

private object StringContextIdent {
def unapply(tree: Ident)(implicit ctx: Context): Option[Ident] = {
if (tree.symbol.eq(defn.StringContextModule)) Some(tree) else None
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline this extractor into StringContextApply


private object StringContextApply {
Copy link
Contributor

Choose a reason for hiding this comment

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

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(strContextApply, List(Literals(strs))), _),
List(SeqLiteral(elems, _)))
if strContextApply.symbol.eq(defn.StringContextModule_apply) &&
elems.length == strs.length - 1 =>
case Apply(Select(Apply(StringContextApply(_), List(Literals(strs))), _),
List(SeqLiteral(elems, _))) if elems.length == strs.length - 1 =>
Some(strs, elems)
case _ => None
}
Expand Down
4 changes: 3 additions & 1 deletion tests/run/interpolation-opt.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@ a1two3.0b

Hello World
Side effect!
Hello World
Foo Bar
Side effect n2!
Titi Toto
5 changes: 4 additions & 1 deletion tests/run/interpolation-opt.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,8 @@ object Test extends App {
println(StringContext(foo, bar).s(" "))
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test and looks like it handles it correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
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(" "))
}