Skip to content
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

Intrinsfy raw and s String interpolators #6093

Merged
merged 1 commit into from Feb 23, 2018

Conversation

@retronym
Copy link
Member

commented Sep 22, 2017

Boxing and varargs incur a significant penalty for string
interpolation, as explored in:

https://medium.com/@dkomanov/scala-string-interpolation-performance-21dc85e83afd

This pull request takes an initial, big step back towards decent performance
by translating calls to these interpolators to string concatenation, which
itself is already translated to string builder appends in the JVM backend.

I have also tried to be a little smarter with the way we size the string
builder.

Longer term, we should build atop of JEP-280 to extract even better
performance with less code for us to maintain. But that's not available
until Java 9.

@scala-jenkins scala-jenkins added this to the 2.12.5 milestone Sep 22, 2017

@lrytz

This comment has been minimized.

Copy link
Member

commented Sep 23, 2017

JavaUniverseForce.scala must be updated.
===========================================================
 DIFF:
===========================================================
--- a
+++ b
@@ -321,2 +321,3 @@
     definitions.StringContextClass
+    definitions.StringContextModule
     definitions.QuasiquoteClass

Also the following, which I cannot pin down by just looking at the jenkins log:

[error] - junit/test
[error]   - junit/test:compileIncremental failed: scala.StringContext$InvalidEscapeException: invalid escape '\x' not one of [\b, \t, \n, \f, \r, \\, \", \'] at index 0 in "\x". Use \\ for literal \.

@retronym retronym force-pushed the retronym:topic/string-s branch from 4dd601f to 49e4a08 Sep 25, 2017

@retronym

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2017

Fixed up the test failure and refactored the implementation some.

I'm happy with the 2.12.5 milestone for this one, seems too risky to rush in to 2.12.4.

@@ -689,6 +689,8 @@ trait StdNames {
val ex: NameType = "ex"
val experimental: NameType = "experimental"
val f: NameType = "f"
val s: NameType = "s"
val raw_ : NameType = "raw"

This comment has been minimized.

Copy link
@dwijnand

dwijnand Sep 29, 2017

Member

alphabetical order?

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

no strong opinion, but it seems the others here are alphabetical indeed.

@retronym retronym force-pushed the retronym:topic/string-s branch from be070a8 to a96e4e2 Dec 1, 2017

@retronym retronym changed the title WIP Intrinsfy raw and s interpolators Intrinsfy raw and s String interpolators Dec 1, 2017

@hrhino hrhino referenced this pull request Jan 9, 2018
0 of 1 task complete

@retronym retronym requested a review from lrytz Jan 15, 2018

@retronym

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

@lrytz Are you happy with the final state of this to target 2.12.5?

@lrytz
Copy link
Member

left a comment

Looks good, only a few cosmetic cleanups.

@@ -1451,6 +1452,10 @@ trait Definitions extends api.StandardDefinitions {
def isStringAddition(sym: Symbol) = sym == String_+ || sym == StringAdd_+

lazy val StringContext_f = getMemberMethod(StringContextClass, nme.f)
lazy val StringContext_s = getMemberMethod(StringContextClass, nme.s)
lazy val StringContext_raw = getMemberMethod(StringContextClass, nme.raw_)
lazy val StringContext_constructor = getMemberMethod(StringContextClass, nme.CONSTRUCTOR)

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

this is unused

@@ -689,6 +689,8 @@ trait StdNames {
val ex: NameType = "ex"
val experimental: NameType = "experimental"
val f: NameType = "f"
val s: NameType = "s"
val raw_ : NameType = "raw"

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

no strong opinion, but it seems the others here are alphabetical indeed.

@@ -322,6 +322,7 @@ abstract class TreeGen {

def mkLiteralUnit: Literal = Literal(Constant(()))
def mkUnitBlock(expr: Tree): Block = Block(List(expr), mkLiteralUnit)
def mkLiteralString(s: String): Literal = Literal(Constant(s)).setType(StringTpe)

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

unused

@@ -1510,6 +1529,39 @@ abstract class RefChecks extends Transform {
currentApplication = tree
tree
}

private object StringContextIntrinsic {
def unapply(t: Apply): Option[(List[Tree], List[Tree])] = {

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

Could return an Option[(List[Literal], List[Tree])] to avoid the casts at the use site, though it would require another map in the raw case. Up to you, both ways is fine.

@@ -1499,7 +1499,26 @@ abstract class RefChecks extends Transform {
isIrrefutable(pat1, tpt.tpe) && (qual.tpe <:< tree.tpe)) =>

transform(qual)

case StringContextIntrinsic(treated, args) =>
var tree: Tree = treated.head

This comment has been minimized.

Copy link
@lrytz

lrytz Jan 22, 2018

Member

maybe call it result to avoid shadowing (and confusion)

@lrytz lrytz added the release-notes label Feb 23, 2018

@lrytz lrytz force-pushed the retronym:topic/string-s branch from a96e4e2 to 72642f6 Feb 23, 2018

@lrytz lrytz self-assigned this Feb 23, 2018

@lrytz

lrytz approved these changes Feb 23, 2018

Copy link
Member

left a comment

pushed my review suggestions

@lrytz

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

/synch

@lrytz lrytz merged commit 9fc1356 into scala:2.12.x Feb 23, 2018

2 checks passed

cla @retronym signed the Scala CLA. Thanks!
Details
validate-main [185] SUCCESS. Took 91 min.
Details
@retronym

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

pushed my review suggestions

Thanks!

@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

questions: this is intrinsics and not a macro in order to get it into 2.12.x, because bincompat...? and the motivation for getting it into 2.12.x instead of just waiting for 2.13.x is compiler performance...?

@retronym

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

Nice, forgot about 475! Thx /cc @hrhino

@xuwei-k

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@eed3si9n

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.