Optimize Build::compile_objects: Only spawns one thread
#780
Conversation
| @@ -19,6 +19,7 @@ edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| jobserver = { version = "0.1.16", optional = true } | |||
| os_pipe = "1" | |||
There was a problem hiding this comment.
The msrv CI for windows failed because it automatically pulls in the latest version of os_pipe v1.1.2, which depends on windows-sys v0.42.
windows-sys v0.42 seems to use if, match, || and && in const, hence failed in rust v1.34.
However, we only need os_pipe v1.0.0 to work here, I've tested this with cargo update --Zminimal-version and all the tests ran just fine.
So I think this does not break the msrv given that the users manage the Cargo.lock to not use the latest version of os_pipe, but not entirely sure about that.
There was a problem hiding this comment.
I agree this doesn't practically update the MSRV. I think a reasonable solution would be to run cargo update -p os_pipe --precise 1.0.0 (possibly after cargo generate-lockfile if needed) in the CI script.
That said, do you know what version these features require? I don't mind bumping it a bit.
There was a problem hiding this comment.
That said, do you know what version these features require? I don't mind bumping it a bit.
I looked into the tracking issue rust-lang/rust#57563 and it seems to be the first item in the list rust-lang/rust#49146 , which is stablised in 1.46.
P.S. If we are bumping msrv to 1.46, then I can replace the Option used in fn jobserver() with mem::MaybeUninit.
There was a problem hiding this comment.
1.46 should be fine. I'll ask if anybody objects on Zulip (https://rust-lang.zulipchat.com/#narrow/stream/351149-t-libs.2Fcrates/topic/Any.20objections.20to.20bumping.20.60cc.60's.20MSRV), but my main goal was to avoid going past Debian stable (which is on 1.48), since it tends to cause a lot of spurious bug reports. Even then, those would probably be directed at the crate that causes the compilation error (in this case windows-sys), rather than us.
That said, I'm slightly unsure about adding the dependency on os_pipe, since it's not nearly as widely used as cc, and is pretty lightweight (so it might be better to implement manually). That said, it might be fine.
There was a problem hiding this comment.
(so it might be better to implement manually).
That's definitely doable, I checked their source code and it is really simple, though I personally don't want more unsafe code in cc.
There was a problem hiding this comment.
Sure, although I think that might be easier unsafe to audit than the current code.
There was a problem hiding this comment.
It's also notable that we don't currently have any non-optional dependencies, and even among optional dependencies we don't have any that aren't maintained by rust-lang team members (this would change that). So I'm not sure, it might be worth being a bit conservative here.
I'm largely inclined to think windows-sys and libc are fine, though.
There was a problem hiding this comment.
So I'm not sure, it might be worth being a bit conservative here.
Hmmm, maybe a good idea to move os_pipe into rust-lang considering that it is also a fundamental lib used by other crates?
CC @oconnor663 owner of os_pipe.
There was a problem hiding this comment.
In any case I still need to review this code and use of the dependency (sadly, it's looking like I might not get to this until the weekend, not sure).
There was a problem hiding this comment.
Happy to help if I can. Luckily the whole crate isn't very much code :)
|
Thanks, this looks great. I'll review more thoroughly later this week, but removing the unsafe alone (without the optimizations) would be enough for me to be very excited about this. |
|
@thomcc Friendly ping just in case you forgot this PR |
|
Ah, sorry, I've been very busy and sadly been neglecting this more than I should. Give me a week or so, I should have some time after that. |
|
@thomcc Pinging as this has been stale for really long time. |
|
@thomcc Pinging since this has been stale for another 2 months. |
|
@JohnTitor Can you review this please if you have time? This PR isn't very complex and it should not take you too much time to review it. |
- Refactor: Extract new fn `Build::compile_object_inner` - Optimize away thread spawning in `Build::compile_objects` Instead of spawning a new thread just to manage the compilation task, we instead spawn the child and put it into a Vec and manage it in the current thread. - Add new optional dep os_pipe v1 - Optimize `Build::compile_objects`: Only spawn one `print` thread instead of one per `Child`. - Rm unused explicit lifetime in `Build::compile_objects` - Optimize `Build::compile_objects`: Do not wait on `child` in `KillOnDrop::drop` - Refactor: Create newtype `PrintThread` - Refactor `spawn`/`run`/`run_output` to use `PrintThread` and extract new fn `run_inner`. - Refactor `Build::compile_objects` to use `spawn` when feat `parallel` is enabled - Fix `spawn`: Ensure stderr is reset on panic - Refactor: Extract new fn `wait_on_child` - Rename `Build::compile_object_inner` => `create_compile_object_cmd` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
|
@thomcc Please review this if you have time, it has been stale for really long time. |
| } | ||
| let child = &mut self.0; | ||
|
|
||
| child.kill().ok(); |
There was a problem hiding this comment.
Nit, but I'm not a fan of using .ok() as an idiom to ignore the return value here. I'd suggest using let _ = and a comment.
(Or, possibly, calling .expect() since it's probably worth killing the process if this fails. But that'd be a semantic change from the previous code, so if done at all that should be a separate PR.)
|
I love this PR. Thank you! I reviewed the code, and both the intent and implementation seems reasonable to me. Given that, and that it passes CI, I'm going to go ahead and merge it. |
|
From zulip:
We should fix this before cutting a release. I'll try to get to it in the next couple days. |
|
This PR seems to be causing build problems with rust-secp256k1. The error message looks like: where the snipped output is a pile of more "cannot find If you compare these two invocations you will see that the latter has I've read the code in this PR a few times and can't see any way that it would cause these linker flags to get dropped, but empirically that's what appears to be happening. |
|
@apoelstra The error log says "linking with cc failed", meaning it isn't executing any cc commands but rather failed linking with cc. Since you are doing a no-std-test, I think this is where the error comes from, since cc relies on linking with libc on unix to work. |
A recent commit (rust-lang/cc-rs#780) broke our CI nightly job. Pin to a version before that was merged. This should be temporary until the issue is fixed.
|
@NobodyXu this makes sense, but is somehow even more confusing than my original theory :(. Why would this PR cause a link error with the |
|
I am similarly not seeing how this PR causes the observed issue. Can you file a new issue so this doesn't get lost? |
This PR optimizes
Build::compile_objectsso that it only spawns one thread for printing warnings for bothcfg(features = "parallel")andcfg(not(features = "parallel")).This PR achieves this by using
os_pipeto create a pair of pipe fds and use that through out the entireBuild::compile_objectsforCommand::stderr.This eliminates the thread spawns for each process just to forward warnings.
The second optimization is specific to
cfg(features = "parallel"), which previously spawns one thread per process creation.I replaced that by simply adopting
Command::spawnand collects theCommand, program_name,process::Childand thejobserver::Acquiredtoken into aVec, then iterate over them.This removes all the
unsafecode in there while also significantly improves performance, especially when the users has a lot of source files but each don't take long to compile (sounds like common cases for me).Signed-off-by: Jiahao XU Jiahao_XU@outlook.com