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

Amortize the startup cost of some components #10451

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Jul 5, 2024

Pull Request Description

Building Engine, Context, ApplicationConfig and Ydoc was a adding a rather large delay during the initial startup step as all of those were blocking operations.
Moving all of those to the resource initialization step hopes to amortize some of that cost since it can be done in parallel. Had to add a ComponentSupervisor (open for a different name suggestion) to ensure that such delayed components are properly closed on shutdown.

Important Notes

Adding Ydoc has added a visible delay during startup. I'm hoping that we can amortize some of that with this PR:
Screenshot from 2024-07-05 11-12-19

Now:
Screenshot from 2024-07-05 11-25-58

and run in parallel:
Screenshot from 2024-07-05 14-03-47

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

Building Engine, Context, ApplicationConfig and Ydoc was a adding a
rather large delay during the initial startup step as all of those were
blocking operations.
Moving all of those to the resource initialization step hopes to
amortize some of that cost since it can be done in parallel.
Had to add a `ComponentSupervisor` (open for a different name
suggestion) to ensure that such delayed components are properly closed
on shutdown.
@hubertp hubertp added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jul 5, 2024
private var component: AutoCloseable = null

def registerService(component: AutoCloseable): Unit = {
assert(this.component == null, "can't register component twice")
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK it works because the single component initialization is guaranteed by the LockedInitialization implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is just a precaution in case someone wants to use in a wrong way.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jul 5, 2024
@mergify mergify bot merged commit 9449bf3 into develop Jul 5, 2024
44 checks passed
@mergify mergify bot deleted the wip/hubert/startup-perf-improvements branch July 5, 2024 11:50
@JaroslavTulach
Copy link
Member

Shouldn't we have a benchmark that would verify you actually speed language server initialization up with these changes? Such a benchmark shall not be too different from this one - just different arguments, right?

@Akirathan
Copy link
Member

Such a benchmark shall not be too different from this one - just different arguments, right?

@JaroslavTulach That benchmark measure startup only of the engine distribution. Whereas, @hubertp was profiling the startup of language server. I am afraid we don't have any benchmarks for language server yet.

@JaroslavTulach
Copy link
Member

@JaroslavTulach That benchmark measure startup only of the engine distribution.

Language server is part of the engine distribution. One just has to use --server option that Runs Language Server.

@Akirathan
Copy link
Member

Language server is part of the engine distribution. One just has to use --server option that Runs Language Server.

@JaroslavTulach That is true. However, for it to be useful, one has to also send some requests, at least acquire capability request (?). If you just start language server by simply running something like enso --server ... --root-id ... <all-the-other-necessary-stuff>, without any client connection, are we sure that the language server will start all its components?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants