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

Support for Java 9 (and 10) #761

Merged
merged 16 commits into from Mar 10, 2018

Conversation

Projects
None yet
2 participants
@robby-phd
Collaborator

robby-phd commented Mar 7, 2018

This pull request addresses #675.

The approach uses https://github.com/retronym/java9-rt-export to extract Java lib class files to ~/.ammonite/rt.jar if sun.boot.class.path is null and use that (and amm) for compilation.
This necessitates Classpath.classpath to know Ammonite's storage backend.

sun.misc.Signal.handlers is no longer available in Java 9 so Signaller is adjusted by maintaining a map of sig handler by itself (please double check).

I copied the code from https://github.com/retronym/java9-rt-export (with its license embedded as comments) directly into the change set of amm/runtime. If undesirable, a different mechanism needs to be decided (the tool is not published in maven).

All automated tests (but one) passed (in my machine); BasicTests.source fails because the rt tool does not export Java lib source files. The tests were run using Zulu JDK 9.0.4.1.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 7, 2018

This looks great; is there any way you can turn on some Java 9 tests in the Travis config, so we can ensure this works going forward?

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 7, 2018

Hmm... amm/test is somehow breaking now... I'm investigating.

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 7, 2018

@lihaoyi I made some more fixes and added stages to run integration and published tests on oraclejdk9.

I had to use sbt to run the tests because published amm does not currently support Java 9.

Status:

  • BasicTests.source still fails on integration/test due to missing Java lib source files as I previously mentioned
  • Some AutocompleteTests.selection.{import, scope, scopePrefix} fail. (I missed running published/test before because it wasn't listed in the Ammonite readme.md.)
    Anyway, can you please take a look at these since you have more insights on how amm autocompletion should work? (I'm actually not familiar with Ammonite; I'm just trying to get mill to run under Java 9 and hoping the changes will work for Java 10 that's coming soon 😃.)
@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 7, 2018

@robby-phd sounds good. Even if autocompletion doesn't work, if you can get most of the tests working that's already a huge step forward.

Feel free to conditionally disable any tests that don't work and we can merge what you have once it's green

Disabled BasicTests.source and some AutocompleteTests.selection.{impo…
…rt, scope, scopePrefix} on Java 9 for now.

@robby-phd robby-phd closed this Mar 7, 2018

@robby-phd robby-phd reopened this Mar 7, 2018

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 7, 2018

@lihaoyi tests are green now.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 8, 2018

Rather than making Classpath.classpath deal with a _classpath global mutable variable, we should properly scope it and manage it's lifecycle as part of the Storage cache, keyed by the current classpath signature (just like our compiled code caches). That means different Repl instances in the same JVM with different storage folders would not share the _classpath/rt.jar, and different Repl instances with the same classpath signatures would share the rt.jar, which is probably what we want. Repls with Storage.Memory should create a new rt.jar every time by default for now.

Other than that everything else looks great!

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 8, 2018

@lihaoyi I have changed classpath and rt to be managed by Storage instead of the global variable.

Unfortunately, there seems to be massive memory leaks when running sbt amm/test and sbt ammRepl/test, which I think are caused by all the rt jars created by many of the tests (using different temp storage backends) that are then dynamically classload-ed but I think somehow keep lingering around.

sbt publish/test actually works on my machine with Java max heap set to 16G; it unfortunately can't be run on travis with the (documented) max 7.5G memory limit.

What do you suggest?

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 9, 2018

@robby-phd the only thing I can think of is that we're forgetting to close some classloader that's keeping all those classes in memory. Could you hook it up to jprofiler and see who's keeping the old classloaders alive in between the tests?

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 9, 2018

@lihaoyi That sounds like another pull request.

For now, I simplified my pull request by not caching rt.jar in Ammonite's storage backend; instead, I modified java9-rt-export's Export to create a temp rt.jar that's reused in the same JVM instance. Classpath.classpath is back now as a val (and I rolled back my changes to {CompilerLifecycleManager, Interpreter, Storage}). published/test now passes using default heap memory configuration.

Does this work for you?

Btw, I've tested published/test under openjdk 10 early-access build 10+46 (also with default heap memory configuration) and it works.

@robby-phd robby-phd changed the title from Java 9 support to Support for Java 9 (and 10) Mar 9, 2018

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 9, 2018

@robby-phd yeah that sounds like a reasonable compromise, will take a look

@robby-phd

This comment has been minimized.

Collaborator

robby-phd commented Mar 9, 2018

@lihaoyi I'm not sure how users typically use Ammonite, but if we're talking about compromise, although not ideal, 2d4d1f seems a better choice performance-wise because it does not create rt.jar every time Ammonite is fired up in a fresh JVM. The global mutable variable was used to simulate Classpath.classpath, which is a global val anyway.

In that version, it just means that the first storage backend that's used to get the classpath is always used to cache rt.jar in the same JVM instance. For most common use cases (i.e., not Ammonite tests), the storage backend is in ~/.ammonite (or ~/.mill/ammonite) which only requires rt.jar extraction once (if the Java version is the same).

Anyway, just a thought.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Mar 10, 2018

looks good to me

@lihaoyi lihaoyi merged commit 819bc80 into lihaoyi:master Mar 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment