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 MillBuildServer #2518

Merged
merged 32 commits into from
May 15, 2023
Merged

Cleanups for MillBuildServer #2518

merged 32 commits into from
May 15, 2023

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented May 13, 2023

  1. Break mill.bsp.worker.State into a separate file

  2. Fold targetTasks into completableTasks, and adjust all callsites to use that

  3. Make completableTasks take a tasks: BspModule => Task[W] parameter, so we can perform all evaluation once up-front and then fall back to normal code after (rather than having it all execute in the context of the evaluator inside tasks, which obfuscates stack traces and evaluation order)

  4. Move agg param in completableTasks to a third parameter list, to allow type inference to work better

  5. Make everything in mill.bsp.worker private

  6. Move all the remaining BSP stuff from main/ and runner/ to bsp/, remove weird reflection hacks to just use direct instantiation where possible since now they're in the same module

  7. remove the BspServerStarter/BspServerStarterImpl layers of indirection

  8. Move BspConfigJson.scala, bspConnectionJson and createBspConnection out of bsp.worker into bsp since it doesn't depend on anything heavyweight that necessitates it being in the worker module

  9. Moved def startBspServer out of mill.bsp.BSP into mill.bsp.BspContext, leaving mill.bsp.BSP to just serve one purpose as mill.define.Module rather than additionally being a grab-bag of helper logic

  10. Simplify the concurrency story around instantiating the BspServerHandle:

    • Previously, the whole of startBspServer was put in a Future, with the main result of the Future waiting on the server to terminate only to do some logging and discard the result, and the creation of the BspServerHandle was propagated via Promises and that the main thread would Await upon
    • With this PR, the main thread runs the main logic of startBspServer, directly returning the BspServerHandle when it completes (or Left[String] on error). Server termination is waited for in a side thread, that does nothing but call listening.get() and do logging when it terminates. We still need a global variable to pass it to def startSession, but this can just be a @volatile var rather than a global Promise
    • This maintains the existing semantics, but using 2 parallel threads instead of 3, with the primary logic shifted to the main thread, and without need for any Awaits

Overall this PR tries to cut a lot of the unnecessary complexity and messiness around the BSP logic. This results in a substantial reduction in lines of code, while still keeping the core logic unchanged. Hopefully this will make things easier to maintain and debug going forward.

Tested manually using VSCode on examples/misc/3-import-file-ivy

@lihaoyi lihaoyi marked this pull request as ready for review May 13, 2023 20:57
@lihaoyi lihaoyi requested a review from lefou May 13, 2023 21:01
@lihaoyi lihaoyi requested a review from lolgab May 14, 2023 15:11
@lefou
Copy link
Member

lefou commented May 15, 2023

For my understanding. How fast is the BSP server responding to the first BSP client initialize request? Does it answer the request in a timely manner, e.g. before a long-compiling build.sc is completely built?

@lihaoyi
Copy link
Member Author

lihaoyi commented May 15, 2023

It's the same on main and on this PR; in both cases: we always start off instantiating BspContext which creates a BspWorker and calls startBspServer with initialEvaluator = None, and later on when bootstrapping completes and an Evaluator is available we call the startSession command which injects the Evaluator into the MillBuildServer for it to use. Once the MillBuildServer is instantiated and registered to Launcher.Builder with launcher.startListening, it should be able to handle the init requests without waiting for the Evaluator to come in AFAICT

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.

Looks good to me!

@lefou lefou merged commit 65fbd53 into com-lihaoyi:main May 15, 2023
@lefou lefou added this to the 0.11.0-M9 milestone May 15, 2023
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