-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Function for Compiling Configuration-Sets in Parallel #41
base: main
Are you sure you want to change the base?
Add Function for Compiling Configuration-Sets in Parallel #41
Conversation
We should probably have both the filtering check AND parallel compilation. It would be good for this PR to update the filtering logic with the scoped threads.
100%, there is no reason to not use this everywhere possible.
Yeah, this was a nightmare yesterday, and exactly what I ran into, so I just fixed the filter. Glad to see I wasn't crazy! It might be worth looking into if |
Yeah, if no one wants this mantle I'll update the extension to use this on Monday.
I'm not opposed to it, but there is a difference here. |
^ Okay, technically, it's not fully synchronous. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not an expert with async rust, but it would be nice if in the future we use tokio
for the thread spawning and management rather than std::thread
since we are using it for all other async code in this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (1)
server/src/session.rs:106
- The new function
compile_everything
should have test coverage to ensure its correctness.
pub async fn compile_everything(&mut self) -> Vec<Diagnostic> {
This PR adds a new
compile_everything
function onSession
: It compiles every configuration set.If there's only one set (very common) we compile it directly.
If there's multiple, we spawn worker threads and compile them in parallel.
Theoretically, this should be very performant, but I haven't actually checked it.
I'm also not sure where we fell on the "should we keep the filtering check".
So, I'm just opening this PR with the function, and I'll let anyone who's interested copy/paste the function and try it out.
At the very least, we could use this for
on_change_configuration
, because we definitely want to compile everything there.Tokio's threads don't play well with the borrow checker. So if you create a tokio thread that borrows something, even if we know that that the thread will be joined before the function ends, the borrow checker assumes the worst case: this thread can outlive the function that spawned it. The mutex locking is only good inside the function, so Rust gets angry if a thread might need it even after the function has returned (and the guard is dropped).
Because this is a general problem, the standard library introduced scoped threads. These are threads which include explicit lifetime tracking, and the life of the thread is tied to the scope that created it (ie. these threads can't outlive their function).
This guarantee appeases the borrow checker which now knows, both the thread and the guard will be dead at the same time: when the function returns.
tokio
might have it's own scoped threads, I'm not familiar. But I don't think we need async await here.Compilation is not an IO bound task. So it would never block anyways.