Startup time improvement v2

[hj-lee]

Hi @holgerbrandl, it is the alternative implementation we discussed on #93.

• Use classloader to run the script main
• Support for -J and -D arguments was added.
• Interactive shell can access functions defined in .kt file or included .kt file

[hj-lee]

I'm not sure why the test failed.

[holgerbrandl]

The failed tests were broken in master as well, because I initially forgot to add gradle to the travis config. Now they should be fine. I'll read through your new PR asap.

[hj-lee]

Hi @holgerbrandl,

Do you have any question or suggestion regarding this PR?

[hj-lee]

I pushed a rebased branch, but the changes are almost identical at least for kotlin files. Thanks to the "Build config refactoring" commit, producing fat-jar has become easier (although the "shadow" plugin has to be upgraded).

[hj-lee]

@abelsromero, "use classloader to run the script main" is the main commit of this PR. "calling directly the kotlin compiler" is a byproduct of it. I'm not sure what classpath pollution you experienced before, but I doubt the commit is related to that. The commit does what 'kotlin' script does by calling the same main class with the same arguments 'kotlin' script would call.

[hj-lee]

@holgerbrandl, @ablelsromero mentioned that many things are present in the same branch. Does it make you hesitate to merge? I'm willing to divide commits or PR requests if that is the case, but to be honest, that would burden me as it would be hard to reconstruct already done. You can tell me the changes you want to merge, and I can arrange that. (Or, github may allow you to choose commits to merge.)

In that reason, I'd briefly explain the commits (focused on the most complex commit "use classloader to run the script main and more") a bit, and look forward to your response. :) Still, I would prefer to reconstruct than leave in the limbo state. Please, don't hesitate to ask, comment, or request if you have any.

Use gradle 4.10.3 : The old gradle wrapper version is not working well on the local machine. I guess it is due to the presence of gradle 5.0 installed, but I don't want to risk it. And, it would not cause any harm.

Disable relocate by changing shadow plugin - The old shadow plugin relocates classes, and it prevents the following commits from working properly. And, again I think no harm would be done.

use classloader to run the script main and more

• The main idea is to let the JVM running kscript run the script class main to eliminate the second JVM start-up.
• It can be done through JarFileLoader and Class.invoke() method.
• -J and -D options were added to allow give JVM argument to kscript JVM, and they are processed in the 'kscript' bash script.
• While reverse engineering 'kotlin' and 'kotlinc' bash script, I thought when compile the script file, it would be better to use the current JVM rather than call evalBash
• KotlinRunner.kt has the kotlin invoking functions.
  -- In case, the script has KotlinOpts, it invoke an additional java or kotlin (depending on the platform)
• AppHelpers.kt and DependencyUtil.kt has refactoring results to share code with other changes
• Kscript.kt
  -- All println has changed to KotlinRunner calls or evalBash calls(--idea and --self-update)
  -- selfUpdate() method was extracted (It was done to help me to understand code better)
  -- Now --interactive classpath contains the compiled script class, too. (It is useful to experiment util.kt or provide kotlin interactive shell with some functionality.)

refactoring - as is. No behavioral change is intended. It may be easier to see the combined diff.

Use java instead of kotlin - Call java instead of kotlin by building a fat kscript jar. kscript is less dependant on 'kotlin' script. Start-up time has reduced a bit further.

[hj-lee]

@holgerbrandl, featurewise, the "use classloader..." commit can be divided to 3 different commits if you don't want some of them.

• use classloader to run main script
• -J and -D options support
• Include script.kt class in the interactive shell classpath

I can add -J and -D option support test cases. If you don't have objection to include the support.

[abelsromero]

I'm not sure what classpath pollution you experienced before, but I doubt the commit is related to that.

By pollution I mean when you think that you are running with a specific lib and in reality it is another. I've seen this in systems that load code dynamically, mainly gradle plugins. And can get messy in scenarios like the deamon, but luckily that does not affect us here...yet.
For instance, what would happen here if you have a script with a dependency already present in the kscript runtime but which has a different version?

[hj-lee]

@abelsromero, I understand. Let me check out properly on the coming days.
I think kotlin runtime would not experience the issue - it should use its own classloader to work properly. In other words, it should find classes among the given classpath argument first. Even though kotlin itself depends on several libraries, it does not suffer the classpath pollution issue.

[hj-lee]

@holgerbrandl, after reading your reply on #198, I experimented with an idea - supporting both println execution and reuse jvm execution by providing an option like --reuse-jvm. Even though I may be able to gather a few anecdotal evidence that the new script execution model does not cause classpath pollution, it is not easy to have enough confidence. So that, having a way to use the old proven execution model, users can experiment with the new execution model by themselves without worries of new model failing their scripts.

With an added support of environment variable such as "KSCRIPT_OPTS" or "KSCRIPT_REUSE_JVM", users of scripts which do not incur issues with the new execution model, would be able to adapt the new model easily.

However, there are a few drawbacks in this approach. It is more complex than only one execution and currently, -J and -D options works only with --reuse-jvm option. To support -J and -D in println mode, more work is necessary.

Anyway, if you think it can be a viable option, please let me know. Then, I will update this PR or make a new PR. It is working, but needs a bit of clean-up.

By the way, one of the main reasons I made this PR is the noticeable start-up time difference on my work Windows machine - it's about 0.6 seconds. (I guess mandatory anti-virus software may contribute to the slow start-up.) If I use kscript only on linux, I wouldn't bother.

(Actually answers regarding your comment on #198 will follow)

[hj-lee]

@holgerbrandl

in presence of KOTLIN_OPTS there is no speed-improvment in #102 I think.

Yes, you're right. The speed gain comes from reusing the JVM which executes kscript itself, but I found no way to change JVM options after it starts. So, to support KOTLIN_OPTS, restart of JVM is unavoidable. (That is the reason I added -J and -D support in the original v1 PR.)

selfUpdate should be removed (to specific and to tied to bash). users willwant to update using their preferred method anyway.

I vote for this change :-) I'm willing to rebase if you made the change.

Use java instead of kotlin - Call java instead of kotlin by building a fat kscript jar.

@hj-lee how does the first part before the dash relate to other second part? I dont' see how your PR would build a fat jar, and I also don't think it's a good strategy because it would make initial script run very slow in case of complex dependencies.

The commit makes kscrip.jar fatjar, not the jar of the given script. I found executing java with fatjar is slightly faster than executing kotlin with normal kotlin jar. In addition, that makes kscript use the compile time kotlin-stdlib instead of run-time kotlin-stdlib - which makes kscript execution more predictable. I guess it has not occurred yet, but imagine kscript uses a new function added in version 1.3.11 stdlib and the user system standard lib is 1.1.50. I put it in this PR as it reduces start-up time a bit, and the only drawback I can find is the kscript.jar size.

cons

• incosistent execution model in presence/absence of KOTLIN_OPTS:

  • first its using spawning a subprocess and
  • second when doing so it using differing interpreters (java->kotlin).

• unclear/potential classpath polution. Will the user code inherit all the kscript dependencies? It must not imho to avoid version conflicts for instance.

• i was happy to have moved all args processing away from shell launcher, now it's coming back

Clearly #102 goes much deeper by changing the execution model. This definitly results in a cleaner design without so many external tool invocations. However, because I care about backwards compatiblity much more than about a slightly faster execution, I don't feel ready yet to merge it because of my concerns/questions listed above). Once these are resolved, #102 is clearly the way forward.

I think I give answers to most of the questions except the classpath pollution one. By digging up Kotlin/Java documents (and/or source code), we may be able to prove the new execution model will not cause classpath collision. But, in my opinion, supporting both the original and the new execution model is more practical approach than putting effort to prove the correctness of the new execution model.

Regarding the other two "cons", it seems they do not affect the kscript users negatively. So, I guess we can accept it :-)

[holgerbrandl]

I'd think that the correct reference is the test-suite. If it passes it, we don't need to find more examples imho. :-)
Maybe to prevent classpath pollution when refactoring kscript's implementation, we could add a simple test like kscript "Class.orName(org.docopt.Docopt)" which must fali.

In general I'd rather do the switch from one exec-model to a better (once we're confident that it keeps backward compatibility). Providing both modes makes it much harder to maintain and test imho. Already reading through PRs and providing supports eats up quite some (personal) time, so I somehow need to keep it manageable.

I have already removed self-update (see referenced commit).

makes kscrip.jar fatjar, not the jar of the given script

Thanks for the explanation. Since kscript is compiled using a rather dated kotlin version, I don't think that this is an issue. Such a change for sake of performance improvements only should be evaluated in a separate patch.

It agree that kscript.jar size is irrelevant.

I think it's very important that user scripts should run against the installed kotlin version and not against the compile version of kscript. This would/might (?) not be the case when using a fat-jar along with java as runtime and with this PR.

However, with the current execution model we could think about using a fatjar+java if it would give a better performance.

we can accept the other cons.

Actually because quite a few features of bash turned out to be pretty platform specific, so getting rid made of bash as much as possible made it much more stable.

---

This PR is sitting there since a while, so we need to come to a conclusion. I highly appreciate your efforts and patience. However, the 2 separate execution models in presence/absence of KOTLIN_OPTS seem too convoluted to me to allow for easy maintenance and testing. So I can't merge it in at the current state. I'm discarding it with a heavy heart, and I'd like to apologize to you for the time and effort you spent on preparing it.

So to allow for a more in-process mode, we would imho rather

• pull out KOTLIN_OPTS upfront and start the process with them. Kscript.kt would simply ignore them.
• consider java + fatjar, while making sure that user scripts run against the installed kotlin instead of the kscript-compile-stdlib.

For sake of simplicity they should go into a fresh PR.

[hj-lee]

How about deprecating KOTLIN_OPTS?

I don't think pulling out KOTLIN_OPTS upfront is a better strategy. I can think of doing it in two ways. One is making bash script grep KOTLIN_OPTS, and the other is reusing kotlin code to process it. But, the latter approach is not viable as it nullifies the startup time improvement. The only viable solution is making bash script check KOTLIN_OPTS, but in the presence of the INCLUDE directive, that will complicate the bash script further and it would become a duplication of kotlin code in bash script.

In my opinion, complex kotlin code is much easier to manage than complex bash script code. The added code, to support KOTLIN_OPTS in the new execution model, is small and isolated - There are only 3 short methods (in total 20 lines of code) and two if statements. Asking to pull KOTLIN_OPTS upfront, is like asking to implement already proven parsing logic in bash (or in some other script language) from scratch.

All in all, if you agree with the above arguments, I'm willing to make a new PR with added tests and improvements. On the other hand, if you are not convinced, I'm ok with closing this PR.

[holgerbrandl]

How about deprecating KOTLIN_OPTS?

First, It's actually a mandatory feature to configure runtime options. In this PR we/you just reimplemented this in bash by with -D and -J. But this level of configuration must be possible from within the script because first, certain shells don't support shebang lines with more than 1 argument. Second, t to allow for interpreter usage (https://github.com/holgerbrandl/kscript#interpreter-usage), we need KOTLIN_OPTS. Third, it's a more consistent configuration approach (given the other directives in kscript).

I totally agree that extracting KOTLIN_OPTS in bash is more tricky/cumbersome and doing things in kotlin is generally easier. That's why I moved away from bash. In fact, the first versions of kscript were written entirely in bash until I realized my mistake after a lot of suffering. :-)

Second, since users (including me have started using it), so pulling it out would break backward compatibility.

but in the presence of the INCLUDE directive,

I never really liked INLUCDE and to me it's a minor feature only. Users should use proper artifacts imho. So I'd think that we could require the presence of KOTLIN_OPTS in the top-level user script. This clearly simplified the process of pulling out KOTLIN_OPTS in bash, but both annotation and old-style comment-directives would need to be supported.

It would be still quite a change to the code-base, but a KOTLIN_OPTS-upfront + in-process launch of the user-script as in this PR could be merged without much headache I think. Before meging, I would probably extend the tests myself to avoid regressions on classpath-pollution and stdout/stdin streaming. Since it's a complex change, we/you could delay additional features (like kotlin -> java, fatjar, etc) to another PR if possible.

Does this make sense to you?

[hj-lee]

Ok, I will try to extract KOTLIN_OPTS processing to kscript bash. (maybe on the coming weekend)

[russelldavis]

You may be interested in #206, which produces a native build using graal, reducing startup time to 0.02s.

[holgerbrandl]

I'm closing this because of inactivity.