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

Jump to bottom

Initialize suggestions database only once #8116

Merged
merged 14 commits into from
Oct 21, 2023
Merged

Initialize suggestions database only once #8116

merged 14 commits into from
Oct 21, 2023

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Oct 19, 2023

Pull Request Description

close #8033

Changelog:

  • update: run language server initialization once
  • fix: issues with async getSuggestionDatabase message handling in new IDE
  • update: implement unique background jobs
  • refactor: initialization logic to Java
  • refactor: UniqueJob to a marker interface

Important Notes

Checklist

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

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 19, 2023
@4e6 4e6 self-assigned this Oct 19, 2023
@somebody1234 somebody1234 added CI: Keep up to date Automatically update this PR to the latest develop. and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Oct 20, 2023
@somebody1234
Copy link
Contributor

just a heads up, it might be useful to merge with develop, there's a fix that should make the lint CI less flaky
(but also, the code formatting CI seems to be flaky on this PR anyway, so might as well fix that first before re-triggering CI)

hubertp
hubertp approved these changes Oct 20, 2023
View reviewed changes
Copy link
Collaborator

@hubertp hubertp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just by reading the code, this looks fine. I'd like to play around with it a bit more in a live setting

.../language-server/src/main/java/org/enso/languageserver/boot/resource/RepoInitialization.java Show resolved Hide resolved
...uage-server/src/main/java/org/enso/languageserver/boot/resource/InitializationComponent.java
boolean isInitialized();

/** Initialize the component. */
CompletableFuture<Void> init();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't init assume that isInitialized is false at the beginning of the execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented it in a higher-level initialization components

...mon/src/main/java/org/enso/interpreter/instrument/command/InvalidateModulesIndexCommand.java
public Future<BoxedUnit> executeAsynchronously(RuntimeContext ctx, ExecutionContext ec) {
return Future.apply(
() -> {
TruffleLogger logger = ctx.executionService().getLogger();
long writeCompilationLockTimestamp = ctx.locking().acquireWriteCompilationLock();
try {
ctx.jobControlPlane().abortAllJobs();
ctx.jobControlPlane().abortBackgroundJobs(DeserializeLibrarySuggestionsJob.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this related to the random deserialization problems you were seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've checked everything. It really shouldn't

@hubertp
Copy link
Collaborator

hubertp commented Oct 20, 2023

🚢 it. I haven't seen any initialization problems anymore.

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Oct 21, 2023
@mergify mergify bot merged commit b1df8b1 into develop Oct 21, 2023
32 of 34 checks passed
@mergify mergify bot deleted the wip/db/8033-initialize-suggestions-database-only-once branch October 21, 2023 20:32
JaroslavTulach
JaroslavTulach reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Many classes were rewritten to Java. Nice.

.../src/main/java/org/enso/languageserver/boot/resource/InitializationComponentInitialized.java
/** Object indicating that the initialization is complete. */
public final class InitializationComponentInitialized {

private static final class InstanceHolder {
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for InstanceHolder when the only method and INSTANCE field of the InitializationComponentInitialized class deal with holding and initializing the instance.

@enso-bot enso-bot bot mentioned this pull request Oct 24, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaroslavTulach JaroslavTulach JaroslavTulach left review comments

@hubertp hubertp hubertp approved these changes

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees

@4e6 4e6

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Initialize suggestions database only once in the presence of multiple clients
4 participants
@4e6 @somebody1234 @hubertp @JaroslavTulach