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

Fix #1524: Implement constant folding for +[string]. #2007

Merged
merged 1 commit into from Nov 20, 2015

Conversation

Projects
None yet
5 participants
@tOverney
Copy link
Contributor

commented Nov 8, 2015

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 8, 2015

Can one of the admins verify this patch?

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

What prevents you from folding classOf[T].toString?

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

There is already an issue for this. Please prefix the commit message with "Fix #1524: ".

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

And as always I forget to write the most important thing first: Thank you!

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

Jenkins, ok to test

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 8, 2015

What prevents you from folding classOf[T].toString?

The result needs to depend on the runtimeClassName function option of the semantics, which is not tracked and difficult to track. Better not to start to depend on that from the optimizer.

@@ -81,6 +81,64 @@ object OptimizerTest extends JasmineTest {

}

describe("String_+ constant folding") {
it("must fold two constant string to the correct value") {
expect("I am " + "constant").toEqual("I am constant")

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

Did you check that this is not simply constant-folded by scalac? I think not, but one way to make sure is to extract one of the two sides in an @inline def.

}

it("must fold the empty string when associated with a string") {
val number: String = "1" + (1.4f).toString.substring(1)

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

It seems that this line is unnecessarily complex wrt. the title of that test. Why not simply "hello" or any other arbitrary string? To prevent it to be known by the optimizer, you can extract it in a @noinline def str = "hello".

}

it("must fold cascading String + correctly") {
def someString(a: Int) = "awesome! " + a + "/10"

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

@noinline if you intend this not to be inlined (maybe one day we will automatically inline small expressions).

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 8, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2395/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3108/
Test FAILed.

def someString(a: Int) = "awesome! " + a + "/10"
expect("ScalaJS" + (" is " + someString(10))).toEqual("ScalaJS is awesome! 10/10")
expect((someString(10) + " is ") + "ScalaJS").toEqual("awesome! 10/10 is ScalaJS")
}

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

What about "ScalaJS" + (1.4f + someString(10))"?

it("must fold integer in double and stringLit correctly") {
expect(1.0 + "hello").toEqual("1hello")
expect("hello" + 1.0).toEqual("hello1")
}

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

Also test an actual integer, like 2.

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

Ah no sorry, that case is later.

it("must fold double with decimal and stringLit correctly") {
expect(1.1 + "hello").toEqual("1.1hello")
expect("hello" + 1.1).toEqual("hello1.1")
}

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

You should also test 0.0, -0.0, Infinity, -Infinity and NaN.

}

it("must fold null and stringLit correctly") {
expect("Damien is not " + null).toEqual("Damien is not null")

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

Are you sure?

:-p

@@ -81,6 +81,64 @@ object OptimizerTest extends JasmineTest {

}

describe("String_+ constant folding") {

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

The "public" name of String_+ is actually +[string]. That is how it is printed by the IR printer. I'd rather you use that name here (and in the commit message).

expect("Damien is not " + null).toEqual("Damien is not null")
}

}

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

It would be good to also have a test with Char. Not sure it will actually constant-fold, but it would be good to guard against a mistake, somewhere.

@@ -2168,6 +2168,18 @@ private[optimizer] abstract class OptimizerCore(
}
}

private def literalToString(l: Literal): String = l match {
case StringLiteral(str) => str
case FloatLiteral(flt) => flt.toDouble.toString

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

This also needs isValidInt something, otherwise 1.0f.toString will badly constant-fold.

case IntLiteral(intgr) => intgr.toString
case BooleanLiteral(bln) => bln.toString
case Null() => "null"
case Undefined() => "undefined"

This comment has been minimized.

Copy link
@sjrd
@@ -2168,6 +2168,18 @@ private[optimizer] abstract class OptimizerCore(
}
}

private def literalToString(l: Literal): String = l match {
case StringLiteral(str) => str

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

In general, we prefer to use the name of the field as declared in the case class unless there is a good reason not to (so value here instead of str).

case (l: Literal, r: Literal) =>
StringLiteral(literalToString(l) + literalToString(r))
case (t1, t2 @ StringLiteral("")) => foldBinaryOp(op, t2, t1)
case (StringLiteral(""), t2) if t2.tpe == StringType => t2

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 8, 2015

Member

Try to stay consistent over the various cases, and put the rhs of the cases on the next line as well.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 8, 2015

"and test it" in the commit message should not be there. If it was not tested, it wouldn't get it in the first place ;)

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

The result needs to depend on the runtimeClassName function option of the semantics, which is not tracked and difficult to track. Better not to start to depend on that from the optimizer.

What do you mean by tracked? If you are talking about incrementality, changing the semantics requires creating a new optimizer anyways.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 8, 2015

Of you're right, I went through that reasoning, now I remember. The real problem is that we also need to know whether the classOf[T] represents a class or an interface. And the optimizer does not have any knowledge query to ask whether a T is a class or an interface.

We could add one, of course, and track it. But it's quite overkill, just to be able to constant fold a classOf[T].toString.

@gzm0

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2015

Ah, nice one. @tOverney please add a comment explaining this. Also, a (doc) comment on literalToString stating that it can't handle class literals would be nice.

@tOverney tOverney changed the title Implement constant folding for String_+ and test it. Fix #1524: Implement constant folding for +[string] Nov 9, 2015

@tOverney tOverney changed the title Fix #1524: Implement constant folding for +[string] Fix #1524: Implement constant folding for +[string]. Nov 9, 2015

@tOverney tOverney force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from ee81a96 to 17b1f3f Nov 9, 2015

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 9, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2396/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3110/
Test FAILed.

@@ -2183,6 +2216,43 @@ private[optimizer] abstract class OptimizerCore(
case _ => default
}

case String_+ =>
(lhs, rhs) match {
case (l: Literal, r: Literal) =>

This comment has been minimized.

Copy link
@sjrd

sjrd Nov 10, 2015

Member

OK now I think I see how you can factor that much more. Instead of defining literalToString(Literal): String, define foldToStringForString_+(tree: Tree): Tree which transforms IntLiteral(v) into StringLiteral(v.toString), for example, and returns tree if it cannot fold the conversion of tree to string.

Then, here, you can just do:

val lhs1 = foldToStringForString_+(lhs)
val rhs1 = foldToStringForString_+(rhs)
(lhs1, rhs1) match {
  ...
}

and now you know that you only have to deal with StringLiterals. Any other stuff that you find is not foldable to a String, and therefore you need not worry about it. Then you will have way less cases here.

(note: from there you can't use default anymore, since at least you need to use lhs1 and rhs1, not lhs and rhs, so defaults should become BinaryOp(String_+, lhs1, rhs1) instead.

@tOverney

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2015

@sjrd @gzm0 Thanks for the feedback, Hopefully I've updated everything.
Regarding the real number printing, @paullepoulpe has an implementation ready here to print real number in the JVM to match ECMAScript specifications.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

That's all. Good job!

@tOverney

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2015

s/ScalaJS/Scala.js

The funny thing is that running that exact regex on the file would break some tests :)
We got the message tho!

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2453/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3179/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2454/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3180/
Test FAILed.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

Scalastyle says:

[info] scalastyle using config /localhome/jenkins/a/workspace/scalajs-task-worker/scalastyle-config.xml
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:144:0: Whitespace at end of line
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:163:0: Whitespace at end of line
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:291:39: Whitespace at end of line
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:294:38: Whitespace at end of line
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:86:18: Public method must have explicit type
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:91:20: Public method must have explicit type
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:102:20: Public method must have explicit type
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:108:18: Public method must have explicit type
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:109:18: Public method must have explicit type
[error] /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/js/src/test/scala/org/scalajs/testsuite/compiler/OptimizerTest.scala:110:18: Public method must have explicit type
@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

Otherwise it's good.
Please also git rebase -i master and squash all the commits into a single commit with the commit message

Fix #1524: Implement constant folding for +[string].

@tOverney tOverney force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from 6264a74 to 4360190 Nov 19, 2015

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2455/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3181/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2456/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3182/
Test FAILed.

@tOverney tOverney force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from f63010f to 7367804 Nov 19, 2015

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

LGTM

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2457/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3183/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2458/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3184/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2459/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3185/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 19, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2460/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3186/
Test FAILed.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

Guess what! Google Closure Compiler wrongly constant-folds Doubles in string concatenation! It emits the JVM version :-p I'll dig if it's a known bug. (Even Rhino does it correctly.)

In any case, you should therefore add a unless("fullopt-stage"). on the test case that is failing. You can see here an example of the coding style we use for unless/when.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

A new entry for the hall of fame: google/closure-compiler#1262

@paullepoulpe paullepoulpe force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from 7367804 to bfbbf7f Nov 19, 2015

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 20, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2461/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3187/
Test FAILed.

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

The bootstrap test fails with the long strings, at the time of deserializing the Trees (so even before we reach the optimizer). I suggest you simply remove the test with long strings. We have enough evidence that the JS VMs can cope with the long literals, since all the "normal" tests passed.

@tOverney tOverney force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from bfbbf7f to b7baee3 Nov 20, 2015

@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

LGTM

@paullepoulpe paullepoulpe force-pushed the SebsLittleHelpers:string-plus-cst-fold branch from b7baee3 to 61fa19c Nov 20, 2015

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 20, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2468/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3195/
Test FAILed.

@scala-jenkins

This comment has been minimized.

Copy link

commented Nov 20, 2015

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/2469/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/3196/
Test PASSed.

sjrd added a commit that referenced this pull request Nov 20, 2015

Merge pull request #2007 from SebsLittleHelpers/string-plus-cst-fold
Fix #1524: Implement constant folding for +[string].

@sjrd sjrd merged commit 4cde878 into scala-js:master Nov 20, 2015

1 check passed

default Build finished.
Details
@sjrd

This comment has been minimized.

Copy link
Member

commented Nov 20, 2015

Yay!

@paullepoulpe paullepoulpe deleted the SebsLittleHelpers:string-plus-cst-fold branch Dec 28, 2015

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