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

[philwalk]

This supercedes #11660

When launching a scala3 script, there are two calls to dist/bin/common, the first from within dist/bin/scala and the 2nd when dist/bin/scala calls dist/bin/scalac.

It turns out that the avoidable 2nd call is quite expensive in terms of what it adds to script startup latency. Here are the reductions in startup time when running in cygwin and in WSL Linux:

On a workstation:
cygwin:    837 mSec
WSL Linux:  66 mSec

On a laptop:
cygwin:  1.03 seconds
WSL Linux: 129 mSec

The numbers are the difference in startup latency for calling a hello-world.sc script with and without the changes in this PR, averaged over 100 runs.

The categories of change in the PR are:

• export variables initialized by dist/bin/common and required by scalac
• modify dist/bin/scalac to conditionally source dist/bin/common
• changes to avoid spurious unset variable errors when testing with: set -o nounset
• fail early if a script file doesn't exist

The third change is exemplified by line 114 in dist/bin/common:

-if [ -n "$CYGPATHCMD" ]; then
+if [ -n "${CYGPATHCMD-}" ]; then

[liufengyun]

@michelou Are you available to review this?

[michelou]

@michelou Are you available to review this?

Yes. I'll have a look (and do some tests under MingW64).

• I assume the most relevant change is lines 28-29 in file bin/scalac.
• I'll need to document myself to check the effects of lines 164-167 in file bin/common.

[michelou]

@philwalk I see no reason to export variable PROG_HOME and function restoreSettings on lines 164-167 in file dist/bin/common.

PS. Personally I would use a function getHome() to initialize variable PROG_HOME (see e.g. file build.sh in my project dotty-examples).

[philwalk]

@michelou

@philwalk I see no reason to export variable PROG_HOME and function restoreSettings on lines 164-167 in file dist/bin/common.

PS. Personally I would use a function getHome() to initialize variable PROG_HOME (see e.g. file build.sh in my project dotty-examples).

I agree, there's no reason to export PROG_HOME or restoreSettings, I'll update the export list.
WRT to how PROG_HOME is initialized, I think at least one of the mingw32 environments (git-bash?) doesn't have readlink.

[michelou]

@michelou

@philwalk I see no reason to export variable PROG_HOME and function restoreSettings on lines 164-167 in file dist/bin/common.
PS. Personally I would use a function getHome() to initialize variable PROG_HOME (see e.g. file build.sh in my project dotty-examples).

I agree, there's no reason to export PROG_HOME or restoreSettings, I'll update the export list.
WRT to how PROG_HOME is initialized, I think at least one of the mingw32 environments (git-bash?) doesn't have readlink.

Good point about git-bash. I'll have to check (I use the getHome() for so many years).

[philwalk]

close-then-open to trigger rebuild due to stalled community build

[liufengyun]

I don't have much expertise on this. The complex exports in bin/common raise a concern about maintainability.

I am wondering if it is possible to avoid calling bin/scalac from bin/scala?

[philwalk]

I don't have much expertise on this. The complex exports in bin/common raise a concern about maintainability.

I am wondering if it is possible to avoid calling bin/scalac from bin/scala?

@liufengyun
I can't see how it would be possible to avoid the call without duplicating the functionality of bin/scalac within bin/scala.
Scripts have traditionally been launched via bin/scala, but in scala3 are executed via bin/scalac.

WRT the exports in bin/common perhaps a test could be written to verify the coordination between scripts.
Another tool that is useful in this regard is the ability to fail a bash script if unset variables are encountered.

[liufengyun]

It seems to me that the two occurrences of bin/scalac in bin/scala don't depend on option parsing in bin/scalac.

If that is the case, then the only significant piece of code is classpathArgs. Is it possible to move classpathArgs to bin/common?

[philwalk]

It seems to me that the two occurrences of bin/scalac in bin/scala don't depend on option parsing in bin/scalac.

If that is the case, then the only significant piece of code is classpathArgs. Is it possible to move classpathArgs to bin/common?

I'll experiment with the idea and report what I find (it might take awhile).
Hopefully the changes to bin/common will not affect the assumptions in bin/scaladoc too much.

[philwalk]

This approach looks promising.

I have a Question for anyone who might know the answer.

What use-case is supported by the following code, which passes arguments directly to the jvm:

In bin/scala line near line 154:

elif [ $execute_repl == true ] || [ ${#residual_args[@]} -ne 0 ]; then
  cp_arg="$DOTTY_LIB$PSEP$SCALA_LIB"
  if [ -z "$CLASS_PATH" ]; then
    cp_arg+="$PSEP."
  else
    cp_arg+="$PSEP$CLASS_PATH"
  fi
  if [ "$class_path_count" -gt 1 ]; then
    echo "warning: multiple classpaths are found, scala only use the last one."
  fi
  if [ $with_compiler == true ]; 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[@]}"
  scala_exit_status=$?

The first test for $execute_repl == true never succeeds (it's dead code) because that state is captured by the previous test.
So the second test is the only way to trigger this code.

Perhaps something like this is intended (it works):

$ scala -run -jar <compiled-script-jar>

or (assuming ./Hello.class exists):

$ scala -run Hello

We might want to add more CLI filtering since, as it's currently configured, any and all unrecognized command line arguments are captured in ${residual_args[@]} and passed to the jvm. That occurs here (near line 113):

    *)
      if [ $execute_script == false ]; then
        # is a script if extension .scala or .sc or if has scala hash bang
        if [[ -e "$1" && ("$1" == *.scala || "$1" == *.sc || -f "$1" && `head -n 1 -- "$1" | grep '#!.*scala'`) ]]; then
          execute_script=true
          target_script="$1"
        else
         # all unrecognized command line arguments are stored here
          residual_args+=("$1")
        fi
      else
        script_args+=("$1")
      fi
      shift
      ;;

  esac

[liufengyun]

@philwalk From the history, it seems come from #3660.

The first test for $execute_repl == true never succeeds (it's dead code) because that state is captured by the previous test.
So the second test is the only way to trigger this code.

Good catch 👍

[philwalk]

I'm getting the following test failure (even with HEAD code, and no modifications):

[info] Test dotty.tools.repl.ScriptedTests.replTests started
expected =========>
scala> val d: java.sql.Date = new java.sql.Date(100L)
val d: java.sql.Date = 1970-01-01
actual ===========>
scala> val d: java.sql.Date = new java.sql.Date(100L)
val d: java.sql.Date = 1969-12-31

I'm not sure what it implies. Happens in Windows 10 cygwin and in WSL ubuntu.

[smarter]

It's probably a timezone issue, feel free to change that test to run (new java.sql.Date(100L)).getTime instead which should always return 100

[philwalk]

Here's what bin/scalac looks like after moving all common code into bin/common .
Let me know if this seems too radical.

#!/usr/bin/env bash

# Try to autodetect real location of the script
if [ -z "$PROG_HOME" ] ; then
  ## resolve links - $0 may be a link to PROG_HOME
  PRG="$0"

  # need this for relative symlinks
  while [ -h "$PRG" ] ; do
    ls=`ls -ld "$PRG"`
    link=`expr "$ls" : '.*-> \(.*\)$'`
    if expr "$link" : '/.*' > /dev/null; then
      PRG="$link"
    else
      PRG="`dirname "$PRG"`/$link"
    fi
  done

  saveddir=`pwd`

  PROG_HOME=`dirname "$PRG"`/..

  # make it fully qualified
  PROG_HOME=`cd "$PROG_HOME" && pwd`

  cd "$saveddir"
fi

source "$PROG_HOME/bin/common"

PROG_NAME=$CompilerMain

prepScalacCommandLine "$@"

# exec here would prevent onExit from being called, leaving terminal in unusable state
eval "\"$JAVACMD\"" "$DEBUG"  "-classpath \"$cp_arg\"" "${java_args[@]}" "${residual_args[@]}"
scala_exit_status=$?

[philwalk]

I checked in the alternate code here PR #12197

[b-studios]

Hey @philwalk, does your alternate PR mean we can close this one?

[philwalk]

Yes, I'm about the commit a final change to #12197, so I'll close this.