Fail jhm on error and fix benchmarks #5020

liufengyun · 2018-08-26T10:47:52Z

#3138 breaks the benchmarks, but it's not catched by the CI due to a missing setting for JMH.

This PR shows the problem and proposes a fix (in the 2nd commit).

The classpath is intended for type checking, not for compiler run-time. Thus, they should be passed as compiler options, not JVM options. It works before because the compiler makes bootstrap classpath available for type checking. However, we can no longer use bootstrap classpath after Java 9 support (scala#3138).

liufengyun · 2018-08-26T11:31:59Z

test performance please

dottybot · 2018-08-26T11:38:03Z

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

dottybot · 2018-08-26T11:48:25Z

performance test failed:

Please check http://lamppc37.epfl.ch:8000/pull-5020-08-26-13.38.out for more information

Blaisorblade · 2018-08-26T13:33:46Z

Was trying to review but was confused by Unrecognized option: -classpath /tmp/1.
That happens because s"-classpath $libs" it taken as one argument and not two (as with "-classpath", libs).

You're right on the rest, tho it's confusing that we allow benchmarks to depend on the whole compiler? If we need that, that's not detected by the tests.

This change

diff --git a/project/Build.scala b/project/Build.scala
index e50bfa733..dabd61558 100644
--- a/project/Build.scala
+++ b/project/Build.scala
@@ -302,7 +302,7 @@ object Build {
   lazy val commonBenchmarkSettings = Seq(
     outputStrategy := Some(StdoutOutput),
     mainClass in (Jmh, run) := Some("dotty.tools.benchmarks.Bench"), // custom main for jmh:run
-    javaOptions += "-DBENCH_CLASS_PATH=" + Attributed.data((fullClasspath in Compile).value).mkString("", ":", "")
+    javaOptions += "-DBENCH_CLASS_PATH=" + Attributed.data((fullClasspath in (`dotty-library`, Compile)).value).mkString("", ":", "")
   )

   // sbt >= 0.13.12 will automatically rewrite transitive dependencies on

changes bench/compile.txt from

../tests/pos/alias.scala
-classpath
/Users/pgiarrusso/git/dotty/compiler/target/scala-2.12/classes:/Users/pgiarrusso/git/dotty/interfaces/target/classes:/Users/pgiarrusso/git/dotty/library/target/scala-2.12/classes:/Users/pgiarrusso/.sbt/boot/scala-2.12.6/lib/scala-library.jar:/Users/pgiarrusso/.ivy2/cache/org.scala-lang.modules/scala-asm/bundles/scala-asm-6.0.0-scala-1.jar:/Users/pgiarrusso/.ivy2/cache/org.scala-lang.modules/scala-xml_2.12/bundles/scala-xml_2.12-1.1.0.jar:/Users/pgiarrusso/.ivy2/cache/org.scala-sbt/compiler-interface/jars/compiler-interface-1.1.6.jar:/Users/pgiarrusso/.ivy2/cache/org.scala-sbt/util-interface/jars/util-interface-1.1.3.jar:/Users/pgiarrusso/.ivy2/cache/org.jline/jline-reader/jars/jline-reader-3.9.0.jar:/Users/pgiarrusso/.ivy2/cache/org.jline/jline-terminal/jars/jline-terminal-3.9.0.jar:/Users/pgiarrusso/.ivy2/cache/org.jline/jline-terminal-jna/jars/jline-terminal-jna-3.9.0.jar:/Users/pgiarrusso/.ivy2/cache/net.java.dev.jna/jna/jars/jna-4.2.2.jar

to

../tests/pos/alias.scala
-classpath
/Users/pgiarrusso/git/dotty/library/target/scala-2.12/classes:/Users/pgiarrusso/.sbt/boot/scala-2.12.6/lib/scala-library.jar

yet benchmarks still seem to work.

dottybot · 2018-08-26T23:58:32Z

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

dottybot · 2018-08-27T02:10:23Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (28ddeac)

liufengyun · 2018-08-27T09:43:24Z

test performance please

dottybot · 2018-08-27T09:43:51Z

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

dottybot · 2018-08-27T11:34:22Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (28ddeac)

Blaisorblade · 2018-08-27T12:45:45Z

bench/src/main/scala/Benchmarks.scala

+    val standard_libs = System.getProperty("BENCH_CLASS_PATH")
+    val compiler_libs = System.getProperty("BENCH_COMPILER_CLASS_PATH")
+
+    val libs = if (args.contains("-with-compiler")) compiler_libs else standard_libs


Is -with-compiler used anywhere? Where would it be used?
If this is used, we should test it in the project/scripts/cmdTests.

Good idea, it'll used in the benchmarks to test "dotty". I'll add it to project/scripts/cmdTests.

Blaisorblade

This looks otherwise OK but I'd like to understand that bit.

allanrenucci · 2018-08-27T13:01:28Z

project/scripts/cmdTests

@@ -12,6 +12,7 @@ EXPECTED_OUTPUT="hello world"
 # check that benchmarks can run
 "$SBT" "dotty-bench/jmh:run 1 1 tests/pos/alias.scala"
 "$SBT" "dotty-bench-bootstrapped/jmh:run 1 1 tests/pos/alias.scala"
+"$SBT" "dotty-bench-bootstrapped/jmh:run 1 1 -with-compiler $($(find compiler/src/dotty -name *.scala -o -name *.java))"


Can we test on a single file? This will be fairly slow

It could be one of the macro test that require the compile on the classpath

liufengyun added 2 commits August 26, 2018 12:45

fail jhm on error

6f55382

This was referenced Aug 26, 2018

Make benchmarks depend only on library classpath #5024

Closed

Don't bootstrap on non-bootstrap CI job #5025

Merged

liufengyun added 2 commits August 27, 2018 11:13

introduce -with-compiler and merge with user -classpath

92f3cd5

suppress summary information to ease parsing of test data

614cf22

liufengyun requested a review from Blaisorblade August 27, 2018 12:01

liufengyun assigned Blaisorblade Aug 27, 2018

Blaisorblade reviewed Aug 27, 2018

View reviewed changes

allanrenucci reviewed Aug 27, 2018

View reviewed changes

test -with-compiler

f247238

liufengyun force-pushed the fix-bench branch from dae1a92 to f247238 Compare August 27, 2018 13:09

liufengyun merged commit ab8bd9e into scala:master Aug 27, 2018

liufengyun deleted the fix-bench branch August 27, 2018 16:07

liufengyun mentioned this pull request Aug 27, 2018

job jobs/work-merge-5023 failed, please handle ASAP. liufengyun/bench#527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail jhm on error and fix benchmarks #5020

Fail jhm on error and fix benchmarks #5020

liufengyun commented Aug 26, 2018

liufengyun commented Aug 26, 2018 •

edited

Loading

dottybot commented Aug 26, 2018

dottybot commented Aug 26, 2018

Blaisorblade commented Aug 26, 2018

dottybot commented Aug 26, 2018

dottybot commented Aug 27, 2018

liufengyun commented Aug 27, 2018

dottybot commented Aug 27, 2018

dottybot commented Aug 27, 2018

Blaisorblade Aug 27, 2018

liufengyun Aug 27, 2018

Blaisorblade left a comment

allanrenucci Aug 27, 2018 •

edited

Loading

Fail jhm on error and fix benchmarks #5020

Fail jhm on error and fix benchmarks #5020

Conversation

liufengyun commented Aug 26, 2018

liufengyun commented Aug 26, 2018 • edited Loading

dottybot commented Aug 26, 2018

dottybot commented Aug 26, 2018

Blaisorblade commented Aug 26, 2018

dottybot commented Aug 26, 2018

dottybot commented Aug 27, 2018

liufengyun commented Aug 27, 2018

dottybot commented Aug 27, 2018

dottybot commented Aug 27, 2018

Blaisorblade Aug 27, 2018

Choose a reason for hiding this comment

liufengyun Aug 27, 2018

Choose a reason for hiding this comment

Blaisorblade left a comment

Choose a reason for hiding this comment

allanrenucci Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

liufengyun commented Aug 26, 2018 •

edited

Loading

allanrenucci Aug 27, 2018 •

edited

Loading