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

Add Function for Compiling Configuration-Sets in Parallel #41

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

InsertCreativityHere
Copy link
Member

@InsertCreativityHere InsertCreativityHere commented Jan 24, 2024

This PR adds a new compile_everything function on Session: 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.

@ReeceHumphreys
Copy link
Contributor

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.

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.

At the very least, we could use this for on_change_configuration, because we definitely want to compile everything there.

100%, there is no reason to not use this everywhere possible.

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).

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 tokio has its own scooped threads just for consistency since we use tokio everywhere else.

@InsertCreativityHere
Copy link
Member Author

It would be good for this PR to update the filtering logic with the scoped threads.

Yeah, if no one wants this mantle I'll update the extension to use this on Monday.

... just for consistency since we use tokio everywhere else.

I'm not opposed to it, but there is a difference here. tokio is all about async/await. It just happens to include some threading, since async/await requires some threading awareness. This function doesn't use any async/await. It's fully synchronous, it just happens to use some worker threads. But unlike async/await, the Rust standard library has pretty decent threading support, without needing another crate.

@InsertCreativityHere
Copy link
Member Author

^

Okay, technically, it's not fully synchronous.
Because at the very top of the function, we lock a tokio mutex...
Other than locking the mutex, it's synchronous though : vP

@InsertCreativityHere InsertCreativityHere marked this pull request as draft January 25, 2024 14:41
Copy link
Contributor

@ReeceHumphreys ReeceHumphreys left a 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.

@externl externl requested a review from Copilot November 19, 2024 20:56

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> {
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