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.

Sign up for GitHub

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

Open
wants to merge 28 commits into
base: 0.12.x
Choose a base branch
from
Open

Use system calls to get terminal size on Linux / Mac #4497

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3548d0a
Use system calls to get terminal size on Linux / Mac
alexarchambault Feb 6, 2025
3932da7
[autofix.ci] apply automated fixes
autofix-ci[bot] Feb 6, 2025
068a0f7
Load jansi native lib ourselves to speed things up
alexarchambault Feb 7, 2025
baea11d
Merge branch '0.12.x' into native-terminal
alexarchambault Feb 17, 2025
e033ef5
Fix coursier interface version
alexarchambault Feb 17, 2025
253cb6b
Fix
alexarchambault Feb 17, 2025
39f2c86
Add missing coursier codecs
alexarchambault Feb 17, 2025
f2fe2d1
Merge branch '0.12.x' into native-terminal
alexarchambault Feb 28, 2025
383cac1
Tweaking
alexarchambault Feb 28, 2025
c2a21c1
[autofix.ci] apply automated fixes
autofix-ci[bot] Feb 28, 2025
ab5829f
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 11, 2025
9a85417
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 12, 2025
c4e8887
Add comments
alexarchambault Mar 12, 2025
302387c
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 13, 2025
1d9cb82
Don't publish jansi in ~/.ivy2/local in integration test
alexarchambault Mar 13, 2025
9ee14f7
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 13, 2025
54b6843
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 14, 2025
16e17be
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 25, 2025
c3625e6
debug
alexarchambault Mar 25, 2025
ee96932
debug
alexarchambault Mar 26, 2025
c525f4a
Adjust logback outfile file upon startup
alexarchambault Mar 26, 2025
ba089f6
debug
alexarchambault Mar 26, 2025
813a8b6
fixup Adjust logback outfile file upon startup
alexarchambault Mar 26, 2025
b80a92d
Merge branch '0.12.x' into pr/native-terminal
alexarchambault Mar 26, 2025
439a9c1
Revert debug stuff
alexarchambault Mar 26, 2025
ad391e1
Make fast path / slow path more explicit
alexarchambault Mar 26, 2025
6802c56
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 26, 2025
bf507e4
debug CI
alexarchambault Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add comments
  • Loading branch information
@alexarchambault
alexarchambault committed Mar 12, 2025
commit c4e888745989ccff79a73552b0fdf9b2f74c4589
8 changes: 8 additions & 0 deletions runner/client/src/mill/runner/client/MillProcessLauncher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ static int getTerminalDim(String s, boolean inheritError) throws Exception {

static {
Copy link
Member

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());
}
}
, where we do a fast-path check whether a cache/marker file exists on disk, and put the complex code inside the conditional so it only runs in the slow path?

Copy link
Collaborator Author

@alexarchambault alexarchambault Mar 12, 2025 •
edited
Loading

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.

Copy link
Member

@lihaoyi lihaoyi Mar 18, 2025 •
edited
Loading

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

Copy link
Collaborator Author

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?)

// Load jansi native library

// First, fast-path code.
// We pre-compute the library location in the coursier archive cache, so that
// we don't need to load the whole of coursier upfront if the native library file
// is already in cache.
Expand All @@ -274,6 +276,10 @@ static int getTerminalDim(String s, boolean inheritError) throws Exception {
archiveCacheLocation,
"https/repo1.maven.org/maven2/org/fusesource/jansi/jansi/" + jansiVersion + "/jansi-"
+ jansiVersion + ".jar/" + jansiLibPathInArchive);

// Seems the jansi native library isn't in cache. We proceed to download it using
// coursier, which is more heavyweight.
// That's the slow path of our jansi-loading logic, that we try to avoid when we can.
if (!jansiLib.exists()) {
// coursierapi.Logger.progressBars actually falls back to non-ANSI logging when running
// without a terminal
Expand All @@ -286,6 +292,8 @@ static int getTerminalDim(String s, boolean inheritError) throws Exception {
jansiLib = new File(
jansiDir, jansiLibPathInArchive); // just in case, should be the same value as before
}

// We have the jansi native library, we proceed to load it.
System.load(jansiLib.getAbsolutePath());

// Tell jansi not to attempt to load a native library on its own
Expand Down
Loading