Enable builds under GraalVM #1266
Conversation
|
I understand the failing tests are the same that were already failing for the master branch. |
|
Thank you for this contribution @ssaavedra ! The build failures are legitimate, see the logs https://travis-ci.org/scalameta/scalafmt/jobs/422630060#L7198 [info] =======
[info] => Diff
[info] =======
[info] -scrut
[info] - .branchSwitch(
[info] - scrut.value,
[info] - casevals = normalcases.map { case (v, _) => v },
[info] - casefs =
[info] - normalcases.map {
[info] - case (_, body) => genExpr(body, _: Focus)
[info] - })
[info] +scrut.branchSwitch(scrut.value,
[info] + casevals = normalcases.map { case (v, _) => v },
[info] + casefs = normalcases.map {
[info] + case (_, body) => genExpr(body, _: Focus)
[info] + }) (FormatTests.scala:81)Tests on master are green https://travis-ci.org/scalameta/scalafmt/builds/420031450 It's only the release step that is failing. I am open to merge this if it enables people to build native images locally. What ordering is used for the new priority queue? iirc, |
|
@olafurpg you are definitely right. Something was to be expected to change, wasn't it? :) Java will probably not honor the Ordered state, I'll take a look at that. |
|
I have just pushed the change for Ordered. Oddly, I think I'm reversing the arguments order. However, this is the order that makes the tests pass (the reverse order produces test errors), so I'm not sure if this is only on my head :) |
scalafmt-core/jvm/src/main/scala/org/scalafmt/internal/PriorityQueue.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you @ssaavedra ! Very cool, change itself looks good to me. I'm happy to merge this if it enables scalafmt with native-image. Before merging I'd like to test it locally myself. I'm currently traveling and busy with deadlines at work so it may not be right away.
Would be great if other people can try out this branch and report how it works :)
This makes it easier to understand that the Ordering is reversed in java.util.PriorityQueue v. scala.collection.mutable.PriorityQueue and makes it less look like a typo.
|
Well, thank you for creating and maintaining it in the first place. I'm ok with further testing. Actually, thanks to Travis and existing tests I have not done the wrong thing already 🙂 I'll tag a v1.6.0-RC4-graalvm1 on my fork in the meantime so that I can push a binary there for the moment. I haven't tried using Travis to build the native-image yet, which could be interesting, but I don't have the time either right now. Please come back with any issues. |
|
Ok, so tried it out, here's what I've found so far (mostly copying from gitter): Some off-the-cuff performance comparisons: This isn't a good test (since the versions aren't the same) but GraalVM managed to format my repo in 13s, vs NailGun at 20s vs plain CLI at 34s. The GraalVM one, however, is master/head (while the other two are 1.5.1) so not an apples to oranges Now, odd thing is that there's now a java process consuming 20% of my RAM. Whatever it is it isn't needed by anything since killing it didn't cause anything to stop working. Running the native scalafmt again did not bring it back. However, running This is all in a T450s ThinkPad running Arch Linux. Besides that, I haven't seen any (obvious) issues so far. |
|
@ssaavedra Push that binary and I'll take it for a spin as well. |
|
@ShaneDelmore it would be good if you can try to build the binary as well, people will likely have to build their own binaries to begin with as it's unlikely we'll manage to configure the CI to build them for us. |
|
It would be good for me to build it, but I'm too busy for that today, but if the artifact is easily retrievable I am not too busy to kick off a background format task. |
|
Just spent a couple of minutes on this. With instructions I can try again but quick test not knowing what I am doing was: Looked at build instructions of native-image -jar scalafmt.jar No luck, but upload a binary or give me a few pointers on how you are building (am I using the wrong scalafmt.jar?) and I will try again. |
|
@ShaneDelmore you want |
|
@pjrt that happens because, by default, And exactly, @ShaneDelmore you want Besides, good news, a static binary for Linux x86_64 is now available at https://github.com/ssaavedra/scalafmt/releases/tag/v1.6.0-RC4-graalvm1 I don't have other kinds of machines to test this with. Also, setting this up for Travis to pick up should not be that much difficult (at least in Linux), it mainly amounts to downloading the GraalVM from GitHub and using native-image from that folder. But for now, it's easier that everyone builds their own for the moment, so that we don't change the scope of this PR. Also, thanks everyone for the engagement! |
|
Looks great. It's slower once JIT kicks in, and slower on file ops, and garbage collection, but still looks like a big win. Running it on 3800 scala files I see: native-image Additionally I see that jvm finishes enumerating files and starts formatting them on my laptop after 12 seconds, native takes 38 seconds to enumerate files before it starts formatting them. When watching it run you can visibly see the files counting by slower, with frequent pauses of <1s. None of this sounds great, except I think most users are formatting a few files at a time, not thousands, and in that case I see very impressive numbers. Running on 22 scala files: native-image Very nice, that's the kind of thing I won't feel bad about calling in a git hook 👍 |
|
Agreed on making building in CI a separate PR, and also that it's not that much work. There is a nice demo from scala days that has scripts that could be re-used for this at https://github.com/graalvm/graalvm-demos/tree/master/scala-days-2018/scalac-native |
|
@ShaneDelmore do you have any insight as of why enumerating could be slower on the native-image? Also, when running the JVM test, were you also using GraalVM's Also, were you using |
|
I was using vanilla jdk, no -git, literally just "scalafmt directoryName". No ideas why file enumeration would be slower. |
|
Using --git 1 does make quite a difference on the large sample it starts formatting almost immediately native or vanilla, but native is still slower eyeballing it largely because it pauses so frequently. Are there default memory usage flags that happen to differ considerably between the two? After adding --git 1 vanilla takes 41s and native takes 1m11s. If all of those pauses were removed I think it would be pretty close. |
|
Maybe JIT is better than native when given enough time? |
|
It might be related to how GC is performed under SubstrateVM. Flags such as Xmx and Xmn can control the amount of memory available. I think you can try to have a larger young generation to avoid so many garbage collections. I am not sure if this must be specified at compile time or at runtime. Still, for the most usual use-case (IMHO) which is formatting changed files ( |
|
~500ms to run Scalafmt on 22 files looks amazing 😍 cc/ @vjovanov any insight on why Substrate underperforms the JVM by ~50% as reported in #1266 (comment) for larger jobs? In particular:
|
|
Part of the slowdown (~2x) comes from using the Graal CE The other part comes from the GC in {{native-image}}: our GC was not designed for throughput. In most of the cases, this does not make much of a difference. We could probably tweak it for this use-case and get decent numbers. In my benchmarks, |
|
Also, in my case, adding Test for garbage collections happening with I couldn't find a way yet to make |
|
I'm not on my laptop right now but are you saying that I can just run And it will know one param is for native-image and the other is for scalafmt? Easy enough for me to test when I get to my laptop if so. |
|
tl;dr: yes. It seems that the |
|
Handy. Played around with settings a bit and got it to run about the same speed as vanilla jdk with a couple GB young gen. |
|
More fun feedback, ee with pgo does indeed beat vanilla jdk on 4000 file test, best run on 4000 files with tweaking different options is 26s, vs 38s for vanilla. Not sure what Oracle's policy is on open source and if ee version would even be allowed to use for an open source project like this, but if so it would be a good default. I can't find anything it doesn't beat the normal version at. |
|
Hi, I also built native image and successfully ran it. In order to confirm that the native image of scalafmt-cli achieved the instant startup and it has acceptable runtime performance, I evaluated the perfomance of three kinds of scalafmt-cli which are all built from current
and ran them on EnvironmentRun on
|
|
How is this proposal going? There seem to be no caveats reported about this change. Should I rebase the proposal onto a single commit? |
There was a problem hiding this comment.
Sorry for the slow responses @ssaavedra I haven't had time yet to try this out myself but the responses from other users in this PR indicate it works well! The diff looks reasonable so let's just give it a go. LGTM 👍
|
If you figure out a good way to install git pre-commit hooks then I think a lot of people would be interested in reading such instructions on the Scalafmt website 😉 |
|
I have a _git_hooks folder under version control, and a "setup-repo.sh" that I call manually, because I don't know how to better automate this on git. Then, my setup-repo.sh reads something like this gist. Then I use |
Is this specific to "userDir" or is it generic for
Any reason to not use the same implementation (the new Scala one) for Scala.js too? Finally, just out of interest, how is it maintained that scalafmt builds on GraalVM? |
This change enables Scalafmt to be built as a
native-imagewith GraalVM from the scalafmt-cli jar.This achieves one of the goals of #1172, namely, avoiding the JVM warm-up time.
For the scalafmt repo, I got a speed-up of 2.5x in my computer. Also, since this method creates a binary just like the current recommended way using coursier and nailgun-ng in the console, it allows for much easier integration with editors such as emacs or vim and other script-based tooling (such as pre-commit hooks).
For building the test native-image I used GraalVM CE 1..00-RC5 under Fedora 28, and I just used
native-image -jar scalafmt.jar.This is the test I performed:
It is important to instantiate
CliOptions()in the main() method instead of using CliOptions.default because the Graal compiler runsAbsoluteFile.userDirat compile-time in order to put the actual value at compilation-time as a constant in the CliOptions.default object. Therefore, in order to have relative paths working again, the CliOptions must be ensured that is instantiated at runtime, and that such constants are not precompiled.The PriorityQueue reimplementation was needed because currently the Scala version of PriorityQueue won't compile into GraalVM due to some calls not being straightforward enough for the static analyzer and some code gets marked as unreachable and is not included in the binary image.