Run test in serial by default to not take all memory #159
Conversation
There was a problem hiding this comment.
Mostly nits, the approach looks good to me, thanks for doing this!
In the future we might have something like a variable to do them in chunks of N or similar, but I agree running all the tests in parallel isn't going to scale well long-term.
IMO, we could get rid of the parallel tests now, and just eventually (and if needed) do the "run N in parallel thing".
Feel free to get rid of that functionality (I think the code might get cleaner), though preserving the functionality after an environment variable works for me too.
| // one. This runs the tests in parallel rather than serially. | ||
|
|
||
| let children: Vec<_> = tests.map(|entry| { | ||
| let children = tests.map(|entry| { | ||
| let child = spawn_run_bindgen(run_bindgen.clone(), bindgen.clone(), entry.path()); |
There was a problem hiding this comment.
nit: can you indent this four spaces to the left while you're here? I know it was wrong before.
| .collect(); | ||
| }); | ||
|
|
||
| let child_wait = |(path, mut child): (PathBuf, process::Child)| { |
There was a problem hiding this comment.
Can you make this a proper nested function (fn child_wait((path, mut child): (PathBuf, process::Child)) -> Option<(PathBuf, process::Child)> { ... }) instead, to reflect it's not capturing any state from the environment?
| if passed { None } else { Some((path, child)) } | ||
| }; | ||
|
|
||
| let failures: Vec<_> = if env::var("RUN_TEST_PARALLEL").is_ok() { |
There was a problem hiding this comment.
Probably better to use a less-generic environment variable, like BINDGEN_PARALLEL_TEST, or something like that?
| // First spawn all child processes and collect them, | ||
| // then wait on each one. | ||
| children.collect::<Vec<_>>().into_iter() | ||
| .filter_map(child_wait) |
There was a problem hiding this comment.
nit: Indent this four spaces, and the similar lines below.
When running all the test in parallel, all the memory on my laptop is consumed by rustc processes, my machine become unusable and the tests do not make progress. It seems to make sense to have serial by default, as the number of process depends on the number of test files.
jeanphilippeD
force-pushed
the
serial_tests
branch
from
a78cb96 to
c82cb6d
Compare
|
Thanks for your comments. I did not know about proper nested function. |
|
@bors-servo: r+ Thanks for doing this! |
|
📌 Commit c82cb6d has been approved by |
|
⚡ Test exempted - status |
Run test in serial by default to not take all memory When running all the test in parallel, all the memory on my laptop is consumed by rustc processes, my machine become unusable and the tests do not make progress. It seems to make sense to have serial by default, as the number of process depends on the number of test files. To run the test in parallel: RUN_TEST_PARALLEL=1 cargo test --feature llvm_stable This is a first approach at this issue, I'm still really new to rust. I was trying to run the test for an easy fix, but it was killing my machine. Ideally it would have been nice to be able to control exactly how many process to run in parallel, maybe using chunks somewhere. I could try to see if I can work it out if it would be a better solution.
When running all the test in parallel, all the memory on my laptop
is consumed by rustc processes, my machine become unusable and the
tests do not make progress.
It seems to make sense to have serial by default, as the number of
process depends on the number of test files.
To run the test in parallel:
RUN_TEST_PARALLEL=1 cargo test --feature llvm_stable
This is a first approach at this issue, I'm still really new to rust.
I was trying to run the test for an easy fix, but it was killing my machine.
Ideally it would have been nice to be able to control exactly how many
process to run in parallel, maybe using chunks somewhere. I could try
to see if I can work it out if it would be a better solution.