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

multithreaded job processing #1311

Merged
merged 11 commits into from
Jul 16, 2023
Merged

multithreaded job processing #1311

merged 11 commits into from
Jul 16, 2023

Conversation

Techatrix
Copy link
Member

@Techatrix Techatrix commented Jul 11, 2023

Independent requests, diagnostics and build file loading will now be processed in parallel. This means faster message handling as well as near instant startup time and more race conditions.

I would appreciate if anyone who reads this would try out this PR to see if there are any issues that need to be addressed.
Also, should multithreading be enabled by default? I already made sure that ZLS can still be build as a single threaded executable.

I've also redesigned the public API of Server that plays nice with zls-as-lib-demo.

Future Improvements:

Tracy is starting to look interesting
Screenshot from 2023-07-11 02-59-06

@FnControlOption
Copy link
Contributor

Currently giving this a try while starting work on peer type resolution in zls; issues I'm having so far:

  • Does not add _ = foo for unused foo
  • VS Code is printing "[Error] Stopping server timed out"
  • No error with -Dsingle-threaded=true

@leecannon
Copy link
Member

Ditto to VS Code printing "[Error] Stopping server timed out" even though it seems to just keep going and works fine.

Seems to be some weirdness with go to in hover, hovering over errorable in the below:

const E = error{ X, Y };
const S = struct { alpha: u32 };

fn errorable() E!S {
    return undefined;
}

On master:
image
This PR:
image

@Techatrix
Copy link
Member Author

Seems to be some weirdness with go to in hover, hovering over errorable in the below:

I overlooked a change while resolving merge conflicts. This should be resolved now

@Techatrix
Copy link
Member Author

  • Does not add _ = foo for unused foo

I've also encountered this issue and though this was a configuration issue. Is is it possible that you are using VSCode and have set zig.astCheckProvider to extension because that causes vscode-zig to set enable_ast_check_diagnostics to false that stops ZLS from generating ast-check diagnostics and therefore won't provide any code actions including autofix.

[Error] Stopping server timed out

I suspect that I am not responding to some request. I will need to do some more investigating.

@FnControlOption
Copy link
Contributor

is it possible that you are using VSCode and have set zig.astCheckProvider to extension

Nope, it's set to zls. It might be that the server simply crashed before it could add _ = foo but I'm not certain 🤔

@leecannon
Copy link
Member

leecannon commented Jul 11, 2023

Sometimes zls stops responding and the server has to be restarted.

Attaching a debugger shows multiple threads sitting here:

const stderr_bytes = try process.stderr.?.reader().readAllAlloc(server.allocator, std.math.maxInt(usize));

After locking up and restarting zls five times:

$ ps -A | grep zig
 122908 ?        00:00:00 zig
 122909 ?        00:00:00 zig
 122952 ?        00:00:00 zig
 122953 ?        00:00:00 zig
 123128 ?        00:00:00 zig
 123129 ?        00:00:00 zig
 123523 ?        00:00:00 zig
 123524 ?        00:00:00 zig
 123584 ?        00:00:00 zig
 123585 ?        00:00:00 zig

@leecannon
Copy link
Member

leecannon commented Jul 11, 2023

The issue occurs when multiple zig child processes are spawned to do ast check at the same time.

I've confirmed that adding a mutex to getDiagnosticsFromAstCheck stops the lockup.

@leecannon
Copy link
Member

leecannon commented Jul 13, 2023

Can't recreate the lockup any more 👍.

I will keep trying to break it but so far this is looking good, also I will hold off on merging other PRs for a while to prevent so many conflicts building up here.

@FnControlOption
Copy link
Contributor

FnControlOption commented Jul 13, 2023

Rebased this branch on commit 952d51b in master and did git merge master: 2a9fe36 (There were no conflicts in src/analysis.zig so you can ignore that file)

@leecannon
Copy link
Member

I haven't been able to break this after ~12 hours of use, so I think its time to merge it and wait for the screams :)

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.

3 participants