avoid duplicate call to dist/bin/common via refactored scalac #12197

philwalk · 2021-04-23T00:15:29Z

This is an alternate fix for script startup latency caused by redundant sourcing of bin/common within bin/scala when launching a script. See #11698 for the original proposal, based on exporting relevant environment variables and conditionally sourcing bin/common.

This PR instead moves additional common code from bin/scalac to functions in bin/common.
The new functions are used to launch the compiler directly from within bin/scala.

In addition, one test was updated to accommodate the change:

dotty.tools.scripting.ClasspathTests

Another test was updated to remove a timezone dependency:

dotty.tools.repl.ScriptedTests

…hTests for refactored bin/scalac

liufengyun · 2021-04-23T06:04:02Z

dist/bin/scala

-    eval "\"$PROG_HOME/bin/scalac\" ${cp_arg-} ${java_options[@]} ${residual_args[@]} -script $target_script ${script_args[@]}"
+    set -- ${cp_arg-} ${java_options[@]} ${residual_args[@]} -script "$target_script" ${script_args[@]}
+    PROG_MAIN=$ScriptingMain
+    prepScalacCommandLine "$@"


I'm wondering what arguments are expected to be parsed here? If it's only 1 or 2, can we do it in bin/scala instead?

I considered trying to do a less extensive change, but ran into problems.

One concern is has to do with alternate ways for launching a script. The following two command lines are currently supported valid ways to launch a script:

scala hello.sc scalac -script hello.sc

These and other similar issues are tested in project/scripts/winCmdTests, and possibly elsewhere.

My assumption is that we want a functionally neutral refactoring, so scripts should use the same default values, for example default_java_opts="-Xmx768m -Xms768m"

They should leverage a single toolchain classpath, and should have access to and respond the same way to scalac options such as -Oshort, for example. So most of what happens in bin/scalac is needed to produce a functionally equivalent java command line.

Anyway, that's what led me to the current implementation, which can no doubt be improved in various ways.

scalac -script hello.sc

I am not sure if Scala 2 supports the above. If not, I think it does not have to be supported.

My assumption is that we want a functionally neutral refactoring, so scripts should use the same default values, for example default_java_opts="-Xmx768m -Xms768m"

This can be either duplicated or moved to bin/common -- maybe duplicating it is easier for maintenance.

They should leverage a single toolchain classpath, and should have access to and respond the same way to scalac options such as -Oshort, for example. So most of what happens in bin/scalac is needed to produce a functionally equivalent java command line.

Given that bin/scala already parses many options that bin/scalac parses, it does not matter to parse 1-2 more options.

Given that bin/scala already parses many options that bin/scalac parses, it does not matter to parse 1-2 more options.

Okay, I see what you have in mind. I'll will review it again.

I am not sure if Scala 2 supports the above. If not, I think it does not have to be supported.

I don't think it's supported by scala 2, although both ways of launching the script must provide -script <scriptname> on the generated java command line, because compiler/src/dotty/tools/scripting/Main.scala line 12 uses it to distinguish jvm parameters from script filename and args.

@liufengyun
Is your idea that we move the command line parsing from bin/common back to bin/scalac and selectively duplicate some of the needed parsing in bin/scala ?

liufengyun · 2021-04-23T06:04:25Z

dist/bin/scala

@@ -165,7 +186,7 @@ elif [ $execute_repl == true ] || [ ${#residual_args[@]} -ne 0 ]; then
    cp_arg+="$PSEP$DOTTY_COMP$PSEP$TASTY_CORE$PSEP$DOTTY_INTF$PSEP$SCALA_ASM$PSEP$DOTTY_STAGING$PSEP$DOTTY_TASTY_INSPECTOR"
  fi
  # exec here would prevent onExit from being called, leaving terminal in unusable state
-  eval "\"$JAVACMD\"" "$DEBUG"  "-classpath \"$cp_arg\"" "${jvm_options[@]}" "${residual_args[@]}"
+  eval "\"$JAVACMD\"" "$DEBUG"  "-classpath \"$cp_arg\"" "${java_args[@]}" "${residual_args[@]}"


Thanks for the refactoring 👍

philwalk · 2021-04-23T14:29:53Z

FYI, this PR resolves a problem in bin/scalac in the way it handles jvm_cp_args, shown here:

# line 79:
  jvm_cp_args="-classpath \"$toolchain\""
# line 101:
  -with-compiler) jvm_cp_args="$PSEP$DOTTY_COMP$PSEP$TASTY_CORE" && shift ;;

In one case, it carries the -classpath prefix, in the other it doesn't. Where it's passed to the jvm on line 129: it was expected to have the prefix. This PR removes the prefix and handles it separately on the jvm command line.

philwalk · 2021-04-23T15:57:05Z

@liufengyun
I have a version that follows your suggestions, and it's now passing tests.
I suspect I should wait to push it until this weekend, to avoid loading the build servers.

liufengyun · 2021-04-23T16:54:19Z

@liufengyun
I have a version that follows your suggestions, and it's now passing tests.
I suspect I should wait to push it until this weekend, to avoid loading the build servers.

Thanks a lot 👍 You are welcome to push whenever it's ready.

philwalk · 2021-04-23T18:50:44Z

I need to manually test all the various existing command line args in bin/scala, such as -repl etc., which will take awhile.
It's passing tests, but I'm not sure how exhaustive the automated testing is.

…ctor and cleanup

philwalk · 2021-04-23T21:45:12Z

The CLI parsing for bin/scalac and bin/scala are now separated again.
This commit also includes a fair amount of refactoring and cleanup, removing unused variables (.e.g, -quiet).

The operating modes for bin/scala were managed by 3 separate mutually exclusive booleans. It now has a single execution_mode string that unambiguously controls which of the 3 modes are selected.

Comments are added to document the purpose of variables, the required order of arguments, and so forth.

Feedback and suggestions are welcome!

liufengyun

LGTM, thank you @philwalk 🎉

Minor: we can replace the two remaining bin/scalac with direct calls, but that can be done in a different PR.

dist/bin/scala

liufengyun · 2021-04-26T15:51:00Z

dist/bin/scala

      # hashbang can combine args, e.g. "-classpath 'lib/*'"
      CLASS_PATH="${1#* *}${PSEP}"
-      class_path_count+=1
+      let class_path_count+=1


Thanks for the explanation above, @philwalk . I'm wondering do we need a || true here and line 74?

line 74 is okay because class_path_count is only zero on line 50, and always > 0 on line 74.
I did discover some other places that did need to be changed:

reference to residual_args in line 160:

if [[ $options_indicator -eq 0 && "${residual_args[@]-}" == "" ]]; then

change "$DEBUG" to ${DEBUG-}" in line 227:

eval "\"$JAVACMD\"" "${DEBUG-}" "-classpath \"$repl_cparg\"" "${java_args[@]}" "${residual_args[@]}"

Thanks for the explanation, @philwalk. Once the two issues you mentioned are addressed, I think we can merge the PR.

philwalk · 2021-04-27T13:11:39Z

Doing manual tests in various shell environments, and if no problems are encountered, I'll push the update.

liufengyun · 2021-04-27T13:18:25Z

dist/bin/common


 # debug

 DEBUG_STR=-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005
+
+classpathArgs () {


Maybe rename it to reflect that it's used for the compiler JVM?

@liufengyun - good idea. I didn't see this until after I pushed changes, so we'll have to do that when we update the two other calls to bin/scalac.

philwalk added 5 commits March 10, 2021 17:52

avoid duplicate call to dist/bin/common on script startup

490b91e

removed unnecessary exports

3984c3a

fix merge conflict: dist/bin/common line 138

c7212b9

refactor all common code in bin/scalac to bin/common

9e3d4f2

update dotty.tools.repl.ScriptedTests, dotty.tools.scripting.Classpat…

a49e578

…hTests for refactored bin/scalac

philwalk mentioned this pull request Apr 23, 2021

avoid duplicate call to dist/bin/common on script startup #11698

Closed

philwalk added 3 commits April 22, 2021 19:38

restore missing main class name to jvm launch command line

c42e314

cleanup inconsistent classpath handling; use all optional scalac args

24ee36e

fix 'bin/scalac' classpath handling regression

2290dbc

liufengyun reviewed Apr 23, 2021

View reviewed changes

restored separate CLI parsing for bin/scala and bin/scalac ; refa…

58f9b23

…ctor and cleanup

liufengyun approved these changes Apr 26, 2021

View reviewed changes

liufengyun reviewed Apr 26, 2021

View reviewed changes

dist/bin/scala Show resolved Hide resolved

dist/bin/scala Show resolved Hide resolved

liufengyun reviewed Apr 26, 2021

View reviewed changes

b-studios assigned philwalk Apr 27, 2021

liufengyun reviewed Apr 27, 2021

View reviewed changes

cleanup gaps in nounset expressions

48262d0

liufengyun approved these changes Apr 27, 2021

View reviewed changes

liufengyun merged commit 01eb5a2 into scala:master Apr 27, 2021

philwalk deleted the refactored-scalac-startup-latency branch April 27, 2021 16:56

liufengyun mentioned this pull request May 4, 2021

Command line options to scala misbehaving #12318

Closed

avoid duplicate call to dist/bin/common via refactored scalac #12197

avoid duplicate call to dist/bin/common via refactored scalac #12197

Uh oh!

Conversation

philwalk commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philwalk Apr 23, 2021 • 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!

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!

philwalk commented Apr 23, 2021

Uh oh!

philwalk commented Apr 23, 2021

Uh oh!

liufengyun commented Apr 23, 2021

Uh oh!

philwalk commented Apr 23, 2021

Uh oh!

philwalk commented Apr 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

philwalk commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philwalk Apr 23, 2021 •

edited

Loading

philwalk commented Apr 23, 2021 •

edited

Loading