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

Cleanups for Mill client #3363

Merged
merged 12 commits into from
Aug 12, 2024
Merged

Cleanups for Mill client #3363

merged 12 commits into from
Aug 12, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Aug 11, 2024

  • ServerFiles.* and OutFiles.* have been turned into constant fields where possible

  • client/ code layout has been overhauled: MillClientMain/MillEnv have been re-organized into MillClientMain, MillLauncher, MillServerLauncher, MillNoServerLauncher

  • MillClientMain.main0, now living in MillServerLauncher.runMain, has been broken down: runMillServer has been extracted out of it, and the Locks/Lock/Locked logic has been tweaked with the addition of TryLocked implements AutoClosable to let us fold some logic into the try-with-resources block

  • Removed the code path for running the Mill server in-process: this cuts down the number of execution modes from 3 to 2, server and no-server. There'll be a bit of performance hit spawning another JVM for the no-server mode, but IMO it's worth it for the simplicity and maintainability of this gnarly code, and is necessary for moving the Mill os.pwd out of the root directory into a sandbox

  • Some preparations for moving the Mill os.pwd out of the root directory into a sandbox, e.g. introducing a MILL_WORKSPACE_ROOT environment variable and plumbing it throughout the codebase

This should help improve the code and implementation quality of the Mill client-server codebase and set the stage for further development

@lihaoyi lihaoyi changed the title [WIP] Run Mill with os.pwd in an isolated sandbox folder Cleanups for Mill client Aug 11, 2024
@lihaoyi lihaoyi requested review from lefou and lolgab and removed request for lefou August 12, 2024 05:39
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of the in-process runner was to retire the different entry points and the script-based decision logic. But it looks like you intend to keep the script logic in the prepended script. Is that so? Wouldn't it be better to optimize the Java code for in-process server and remove all the script logic?

@@ -2,7 +2,7 @@

class FileLocked implements Locked {

private java.nio.channels.FileLock lock;
protected java.nio.channels.FileLock lock;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to expose this mutable variable? Can we make it final or provide a protected getter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opening a separate PR to finalize this and a bunch of other things that probably should be final

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lihaoyi
Copy link
Member Author

lihaoyi commented Aug 12, 2024

I think we'll likely always have a client server architecture for performance. Keeping JVM compiler objects hot and jitted requires them to be long lived. The only question is where you draw the line:

  1. Thick JVM client, thin JVM server (e.g. Bazels one-process-per-worker architecture)
  2. Thin bash launcher, thin JVM client, fat JVM server (where we are now)
  3. Thin JVM client, fat JVM server (possible with graal native image or scala native)
  4. Thick bash client, fat JVM server (this is the only scenario where we have a single JVM running everything in process)

We are now at (2). I think trying to get to (3) makes the most sense. (1) could work, but the overhead of short lived thick JVMs is significant even if the heavy lifting compilers etc are kept alive elsewhere. (4) would probably be an unreasonable amount of complexity to implement in bash

Given the idea to move from (2) to (3) above, this PR removes the inprocess mill execution code path from the client, since we'll always have two JVMs for the reasons above

@lihaoyi lihaoyi merged commit a097392 into com-lihaoyi:main Aug 12, 2024
30 checks passed
@lefou lefou added this to the 0.12.0 milestone Aug 12, 2024
lihaoyi added a commit that referenced this pull request Aug 12, 2024
follow up for #3363

* Remove the `public`-ness from `FileTryLocked` and `MemoryTryLocked`.
They do not need to be public, as people should only access them through
the `TryLocked` interface

* Add `final` to the fields that do not need to be mutated (i.e. all of
them)

* Make `Locks` `final`, and replace weird anonymous class initializers
with a proper constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants