-
-
Notifications
You must be signed in to change notification settings - Fork 393
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
Use system calls to get terminal size on Linux / Mac #4497
base: 0.12.x
Are you sure you want to change the base?
Use system calls to get terminal size on Linux / Mac #4497
Conversation
if (mill.main.client.Util.hasConsole()) | ||
try { | ||
NativeTerminal.getSize(); | ||
canUse = true; | ||
} catch (Throwable ex) { | ||
canUse = false; | ||
} | ||
else canUse = false; | ||
|
||
canUseNativeTerminal = canUse; | ||
} | ||
|
||
static void writeTerminalDims(boolean tputExists, Path serverDir) throws Exception { | ||
String str; | ||
|
||
try { | ||
if (java.lang.System.console() == null) str = "0 0"; | ||
if (!mill.main.client.Util.hasConsole()) str = "0 0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we're checking mill.main.client.Util.hasConsole()
twice here; once in the static
blockl and once in writeTerminalDims
. We can probably skip one of those checks right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasConsole
caches its result now, so calling it twice shouldn't incur any cost
Trying this out on my macbook seems to add about ~200ms latency (~150ms -> ~350ms), running |
I'm getting the same numbers :/ Printing the duration of some of the calls to get the terminal size, I get things like
So once the native things are setup, getting the terminal size is sub-ms. Upon initialization, jansi reads a binary file (like |
If the resource/tempdir stuff is circumventable, does that mean that it isn't necessary for the jni functionality? |
Not necessarily, we can load the native library from elsewhere on disk if we want to. I just pushed code that loads it via the coursier cache (this needs coursier/coursier#3272, so this is going to need new coursier and coursier-interface releases). If the native library is already in cache, there's no need to write a temporary file. From native images, we can also statically link the native library, so that nothing needs to be loaded at runtime. But this needs the JNI library to push static libraries somewhere (like here or here), and jansi doesn't. It worked fine for other JNI libraries in Scala CLI, although I haven't seen that being done elsewhere. |
From Java 22 onwards, it should be possible to use FFM, JLine is able to for sure. This doesn't need to load custom native libraries upfront (although I have very little experience with it for now). |
Unpacking the native library and caching it via coursier sounds reasonable. We just need to make sure Coursier is not on the hot path so we don't need to classload all the coursier/scala code when the library is already unpacked. |
(Seems this is surfacing issues with newer versions of coursier) |
Conflicts: build.mill scalalib/src/mill/scalalib/JsonFormatters.scala
I'm having trouble trying to
|
I have a Mac ARM too, and I'm getting no issues. Could you have jansi in your What does |
Hah, indeed I have a |
Seems to work now! |
@@ -248,15 +250,81 @@ static int getTerminalDim(String s, boolean inheritError) throws Exception { | |||
|
|||
private static AtomicReference<String> memoizedTerminalDims = new AtomicReference(); | |||
|
|||
private static final boolean canUseNativeTerminal; | |||
|
|||
static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is pretty messy, and I'm wondering if there's some way we can make it obvious that the fast path is fast and does not classload any heavy libraries. That is the case now, but the fact that it's not obvious from the code makes it likely to regress accidentally if people touch this code.
Perhaps we could do something like what we do for the JVM versions in
mill/runner/client/src/mill/runner/client/MillProcessLauncher.java
Lines 134 to 152 in 392f32d
if (jvmId != null) { | |
// Fast path to avoid calling `CoursierClient` and paying the classloading cost | |
// when the `javaHome` JVM has already been initialized for the configured `jvmId` | |
// and is ready to use directly | |
Path millJavaHomeFile = Paths.get(".").resolve(out).resolve(millJavaHome); | |
if (Files.exists(millJavaHomeFile)) { | |
String[] savedJavaHomeInfo = Files.readString(millJavaHomeFile).split(" "); | |
if (savedJavaHomeInfo[0].equals(jvmId)) { | |
javaHome = savedJavaHomeInfo[1]; | |
} | |
} | |
if (javaHome == null) { | |
javaHome = CoursierClient.resolveJavaHome(jvmId).getAbsolutePath(); | |
Files.createDirectories(millJavaHomeFile.getParent()); | |
Files.write(millJavaHomeFile, (jvmId + " " + javaHome).getBytes()); | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to try to make that clearer from the code, but it already goes first through a fast path, then through a slower one. The fast path goes from the beginning of the static {
block up to File jansiLib = …
. It calls a method on coursier.paths.CachePath
, but it's a fast thing, that shouldn't load much more coursier classes (the method is here, and it loads that class, which itself loads directories-jvm classes - it's all pure Java classes). It does all that to compute the coursier "archive cache" location (where it unpacks archives), and the location of the jansi dynamic library inside it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I'm aware it currently goes through all lightweight JVM classes, and the manual startup-time benchmarks verify it. But we should refactor the code to make it obvious at a glance at a single if-conditional rather than having to read through 70 lines of imperative code and dig through third-party libraries, otherwise it is prone to regress when someone overlooks it and makes changes later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just put the loading logic aside in a separate class, with two methods, tryLoadFast
and loadSlow
(did you mean that kind of thing?)
Added some rough measurements and this cuts down the time for |
Seems this puts jansi in |
Conflicts: integration/feature/init/src/MillInitMavenTests.scala
It seems these consistently fail on CI, but not locally, I'm not sure why
|
Not sure, but you can use the commented-out |
This makes Mill use my native-terminal library (formerly windows-ansi) to get the terminal size. That way, Mill doesn't run
tput
commands every ~100 ms to query the terminal size.