zeroc-ice / vscode-slice

Slice syntax highlighter for Visual Studio Code
BSD 3-Clause "New" or "Revised" License
5 stars 4 forks source link

Add Function for Compiling Configuration-Sets in Parallel #41

Open InsertCreativityHere opened 9 months ago

InsertCreativityHere commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

^

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