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

Less user-facing dependencies via classloader isolation #941

Merged
merged 14 commits into from Jun 28, 2019

Conversation

Projects
None yet
2 participants
@alexarchambault
Copy link
Collaborator

commented Mar 14, 2019

This PR manages to isolate the core modules and dependencies of Ammonite (repl, interp, upickle, fastparse, …) from the user-facing API. Users effectively don't see the former, and can even load other versions of those if they want.

It moves the user-facing parts of interp and repl to new modules interp-api and repl-api.

It then manages to:

  • launch Ammonite and its tests with a two-level classloader hierarchy (first classloader loading the user-facing things - interp-api, repl-api, etc., second one the rest, including the core of Ammonite), by using a custom test runner for the tests, and by packaging Ammonite via a coursier bootstrap rather than an assembly,
  • have Ammonite pick the first classloader to later compile and load user code. This is done by default in the tests, and when one passes the --thin option to the Ammonite launcher.
@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2019

Overall, this allows not to expose plenty of dependencies, most notably fastparse, upickle, jline, etc. Users can in turn load their own versions of those if they want to.

This diff shows which ones are removed:

$ diff <(coursier resolve com.lihaoyi:ammonite_2.12.8:1.6.4-6-b7dafbe5 |sort) <(coursier resolve com.lihaoyi:ammonite-repl-api_2.12.8:1.6.4-6-b7dafbe5 |sort)
1d0
< com.github.javaparser:javaparser-core:3.2.5:default
3d1
< com.lihaoyi:acyclic_2.12:0.1.5:default
5d2
< com.lihaoyi:ammonite-interp_2.12.8:1.6.4-6-b7dafbe5:compile
8,10d4
< com.lihaoyi:ammonite-repl_2.12.8:1.6.4-6-b7dafbe5:compile
< com.lihaoyi:ammonite-runtime_2.12:1.6.4-6-b7dafbe5:compile
< com.lihaoyi:ammonite-terminal_2.12:1.6.4-6-b7dafbe5:compile
12d5
< com.lihaoyi:ammonite_2.12.8:1.6.4-6-b7dafbe5:compile
14d6
< com.lihaoyi:fastparse_2.12:2.1.0:default
18,25c10
< com.lihaoyi:scalaparse_2.12:2.1.0:default
< com.lihaoyi:sourcecode_2.12:0.1.5:default
< com.lihaoyi:ujson_2.12:0.7.1:default
< com.lihaoyi:upack_2.12:0.7.1:default
< com.lihaoyi:upickle-core_2.12:0.7.1:default
< com.lihaoyi:upickle-implicits_2.12:0.7.1:default
< com.lihaoyi:upickle_2.12:0.7.1:default
< com.lihaoyi:utest_2.12:0.6.4:default
---
> com.lihaoyi:sourcecode_2.12:0.1.3:default
29,33d13
< net.java.dev.jna:jna:4.2.2:default
< org.javassist:javassist:3.21.0-GA:default
< org.jline:jline-reader:3.6.2:default
< org.jline:jline-terminal-jna:3.6.2:default
< org.jline:jline-terminal:3.6.2:default
38d17
< org.scala-sbt:test-interface:1.0:default
@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2019

Ultimately, after some more clean-up of user facing stuff, we could enable mima on the user-facing modules, to ensure the API exposed to users doesn't change much.

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch 6 times, most recently from 8d8e9eb to 166b6e2 Mar 15, 2019

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

So a bit of explanation. This PR does two things:

  • add new interp-api and repl-api modules, aimed at being user-facing, while interp and repl should be hidden from users.
  • manage to actually have only those modules (and their transitive dependencies) be exposed to users, both in the tests and when using the Ammonite launcher.

The first point mostly moves things around (f0e41d9, 0d6c3ed, dd12153, 5e168d5, 972562a). It also introduces new bridges (ecc100b, 834fc4b), so that the internals and the dependencies of the frontend and source-related stuff can be hidden too (users only interact with these APIs, rather than by directly tangling with some internal stuff).

The second point is also two-fold, and some aspects can probably be tweaked or even done other ways. It consists in ensuring, both in the tests and with the Ammonite launcher, that Ammonite is started with a two classloader setup (with a first classloader with the interp-api and repl-api modules and their dependencies, and a second classloader with the other modules and dependencies, having the first classloader as parent). It then has the first classloader be picked up as a start, when compiling and classloading user code (that way, things from the second classloader are effectively hidden from users).

Getting a two classloader setup in the tests is done by commit 0bb3b6e. This relies on lihaoyi/mill#572. This basically passes the repl-api classpath (more exactly the test-api classpath…) via a Java property to the test runner, that then creates the two classloaders itself.

Getting those two classloaders with the Ammonite launcher is done in 238c166. This basically replaces the assembly, which merges all dependencies together, by "bootstraps" from coursier. Bootstraps embed the various dependency JARs as resources, along with files listing which ones should be loaded together (see mill -i 'amm[2.12.8].bootstrap' && unzip -l out/amm/2.12.8/bootstrap/dest/bootstrap.jar, things under coursier/bootstrap/launcher/jars/). They contain a tiny Java program, able to read those files, create the corresponding classloaders using the JARs from the resources, and run a main class from them.

cf5405a then ensures that compilation and classloading of user code starts from the classloader of repl-api, set up above. (In the tests, this is done by 0bb3b6e, look around Frame.createInitial).

Lastly, 4631aac, 35389c3, and 166b6e2, do minor refactorings that make things easier for other commits here. And 19f012d proceeds with hiding the upickle dependencies from users, by moving bits of the (user-facing) util module to (non user-facing) runtime.

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

Overall, this adds a bit of complexity (new modules, only a subset of those are accessible to users, classloader hacks in tests and in the launcher). My opinion is it's worth it… This should make the user-facing stuff of Ammonite thinner, and hopefully more stable (in the sense "less changing"). Which should in turn make scripts (and, hmm, notebooks) easier to maintain.

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

@lihaoyi If you're ok with the general idea, I can totally split that PR, for easier review:

  • the minor refactorings of 4631aac and all first,
  • adding interp-api and repl-api,
  • picking the right classloader if it's there, cf5405a,
  • two classloader setup in the tests,
  • two classloader setup in the launcher,
  • no more upickle in the user-facing API.
@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

Lastly, about the bootstrap stuff replacing assemblies, I went for that as I'm familiar with bootstraps (and they have the advantage of not tangling with bytecode too). But I guess using stuff like jarjar to just rename a bunch of classes (those not accessible to users) could work too.

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Mar 20, 2019

@alexarchambault I'm fine with the general idea, but this is a large PR and I might take some time to get around to reviewing it. Hope that's ok!

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 20, 2019

Yep, no problem! (This doesn't block anything on my side…)

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2019

@alexarchambault I skimmed this, it looks good to me. My one request would be to make the classpath-isolation configurable, and maybe opt-in from the command line via a flag in Cli.scala. I think having the messy classpath is a reasonable default for "casual" usage.

My own usage involves files/subprocesses/http/json reasonably heavily even if it isn't necessary for ammonite's own user-facing API, and if want to use Ammonite as a tool for learners/small-projects then the benefit of having all those utilities already bundled outweighs the costs of a slightly polluted classpath.

Another place where the classpath pollution is helpful is when embedding Ammonite in a larger application: in this scenario having access to all your application classpath is the point of the embedding.

Advanced users like yourself who want a cleaner classpath to better control the runtime environment can then just flip the switch.

If you can get tests passing, I think I would be happy to merge this

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch 4 times, most recently from 18258a5 to 00ca1ae Jun 25, 2019

Enable classloader isolation in tests
Moves stuff available from the test sessions to a new test-api module in
particular.

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch 2 times, most recently from d912777 to 42d950e Jun 25, 2019

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

There's still some slight perf regression due to the use of "bootstraps" (that embed the dependencies as resources, so we have "jar-in-jars") instead of assemblies (with no such indirection). I have some workarounds in the making…

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Jun 26, 2019

Sounds good!

Before merging, also please update the PR description with a more verbose description of the significant changes in the PR. Ideally someone reading would be able to get a good summary of the changes without wading through the large number of changed files

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch from 42d950e to e804be7 Jun 27, 2019

alexarchambault added some commits Mar 13, 2019

More loose check
A new line can be inserted right after "default" if its line is too
long.

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch from e804be7 to 4d459da Jun 27, 2019

@alexarchambault alexarchambault force-pushed the alexarchambault:api branch from 4d459da to 2fd9eaf Jun 27, 2019

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2019

Ok, I updated the description.

@alexarchambault alexarchambault merged commit 9ada29c into lihaoyi:master Jun 28, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexarchambault alexarchambault deleted the alexarchambault:api branch Jun 28, 2019

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

@alexarchambault I've noticed some performance regressions that I think arise from this PR. The default REPL (without --thin) is definitely much less responsive than it used to be. Any idea if this could have caused that? We should probably sort that out before tagging another stable release

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

Now that we have this infrastructure in place, here is a bunch more packages it might make sense to keep out of the --thin classpath:

  • shapeless & argonaut; maybe we could make coursier us uPickle instead, since we already use it heavily in the rest of Ammonite/Mill?

  • jline? Not sure how easy this would be to remove, but I don't think a user ever needs this directly (only transitively through the JLineFrontEnds)

  • coursier/internal/shaded/fastparse and coursier/internal/shaded/sourcecode: we already use the libraries in Ammonite anyway, so coursier could just depend on those to avoid duplication

  • scala-compiler: we need this to compile stuff, and the user has access to it through some of the interp.* and repl.* APIs, but it's conceivable we could remove it if we were willing to provide that functionality through a narrower interface

os-lib/pprint/sourcecode seem difficult to remove, since we expose os.Paths in many APIs and want to allow a user to add custom pretty-printers.

coursier.bootstrap.launcher seems like something a user shouldn't need to interact with, but also not clear if it's feasible to remove

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 30, 2019

@lihaoyi You're probably seeing the same kind of slowdown I saw before https://github.com/lihaoyi/Ammonite/pull/941#issuecomment-505542171… I mostly fixed it when --thin is passed, but there's still a penalty if it's not.

The old assembly logic is still around if needed (mill 'amm[2.13.0].assembly'). I'm fine releasing the assembly until this is sorted out. I can open a PR doing that.

I'll add a lengthier explanation here tomorrow about how the "bootstrap" works, to give a better idea about what (I think) is going on.

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

About hiding more stuff when --thin is passed:

  • shapeless and argonaut (and argonaut-shapeless) are brought by coursier. I'd like to switch Ammonite over to the Java API of coursier, that has no dependencies (but still has some of the same feel as the scala API). That'll remove those dependencies.
  • jline is brought by scala-compiler (coursier resolve com.lihaoyi:ammonite-replApi_2.13.0:1.6.9 --what-depends-on jline:jline), it can probably be manually excluded in a first time.
  • the coursier/internal stuff correspond to shaded libraries. I'm not sure these can be "unshaded", unless I publish some coursier modules where fastparse isn't shaded. I'd prefer to go the other way around actually, and have Ammonite rely on a coursier module shading more stuff, via its Java API (see above). Plus these are under a namespace marked as internal, and I don't think these are usable anyway (shading doesn't adjust the "scala annotations", that still refer to non shaded namespaces, making compilation fail when code refers to those)
  • scala-compiler could probabaly be hidden by reworking a bit the API, yeah

I think os-lib could be removed too. I managed to do it locally some time ago, that requires changing some os.Path to java.nio.file.Path in the API in particular.

@lihaoyi

This comment has been minimized.

Copy link
Owner

commented Jul 2, 2019

Swapping some of the APIs to java.nio.file.Path sounds reasonable. I'd say we add the new java.nio based APIs to the bridge traits, and add a new bridge trait that only gets used in --thin that only has java.nio

@alexarchambault

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

Couldn't an implicit os.Path to java.nio conversion do? Enabled only when --thin is not passed.

If necessary, it could even be restricted to the API calls, with some indirection (like defining a implicit case class Path(path: java.nio.file.Path) in ammonite.repl.api, having the api accept ammonite.repl.api.Paths, and add an implicit os.Path to ammonite.repl.api.Path conversion when --thin is not there).

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.