Conversation
|
Oh, hmm, I see that there are ci issues because of rayon's dependencies having build scripts(outside of the two non-deterministically failing tests I had mentioned). I don't think there is much I can do there. I would hope this isn't an issue since rayon is a very popular and well-maintained but I understand if an exception cannot be made. |
|
Thanks for the PR!
It is not very important whether the dependencies have build scripts or not, as they are only used as a reference when auditing dependencies. The error can be suppressed by adding the dependency name to the following list: Lines 22 to 25 in 74823c8 |
| progress: &mut Progress, | ||
| progress: &Arc<Mutex<Progress>>, | ||
| ) -> Result<()> { | ||
| let mut progress = progress.lock().unwrap(); |
There was a problem hiding this comment.
I believe this guard is held until the execution of the command is completed, so I'm concerned that this may cause the executions to not be parallel.
There was a problem hiding this comment.
I added some lexical scoping to limit how long these locks are held. This did involve moving the count increment because of the off chance that 2 threads would increment before either got to print their count. In general, I'm not sure how to look for more cases of this forced serialization.
mattfault
left a comment
mattfault
left a comment
There was a problem hiding this comment.
Looks good! Some small nits about the synchronization. Other than that I am looking forward for this to go in :)
| } else if cx.parallel { | ||
| cx.target.par_iter().try_for_each(|target| { | ||
| let mut line = line.clone(); | ||
| line.arg("--target"); | ||
| line.arg(target); | ||
| packages.iter().try_for_each(|pkg| { | ||
| exec_on_package(cx, pkg.id, &pkg.kind, &line, progress, keep_going) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Minor nit: I think it might be better to nest this in a larger else clause that catches the entire !target.is_empty() case, rather than three individual cases of empty, non-empty parallel, non-empty non-parallel.
Pat-Lafon
force-pushed
the
multithreaded2
branch
from
68ef8d7 to
14bcb62
Compare
|
@m-esposito I rebased on main and took your suggestion for The if statement still has 4 branches regardless of how I reorganize it no? I guess I don't mind but it doesn't seem like much of an improvement to me. Do you have any suggestions for actually verifying that code is running in parallel as expected and isn't blocked in some way? I don't have much context for this code but I would imagine that is the main blocker |
|
Hm, re the About the benchmarking, I did some preliminary testing and it seems like it's not running in parallel. Cargo invocations can't run in parallel without splitting away the target directories (noticeably when you save in an IDE w/ rust-analyzer, with check enabled on every save, then you try |
|
Closing per #128 (comment). Thanks anyway for the PR! |
|
Wonderful! |
3 participants
Hello! I've been occasionally interested in #180 which has been stalled for at least a year. I think I now have a little bit of time/interest to try this. I'm not super familiar with this code base nor in making code parallel so I would appreciate any feedback.
I'm starting from the same idea that #180 created except I am following the suggestion of using a runtime flag instead of a compile time feature flag to enable parallelism. Because of the non-trivial changes that have occurred on main, I have started from scratch instead of rebasing into #180... but all credit should go to @NishantJoshi00
I have done roughly the minimal amount of work to get started so I can get help addressing the three open questions as I understand them:
TargetDirPoolwas used in the previous attempt. I haven't added it yet since I'm not sure about the interactions here but it should be trivial to add if it is still useful/the right approach.