-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Run merged doc-tests by default in the same process and add opt-out attribute #129119
Comments
It's not the only reason. It's because it changes the output and adds a new code example attribute.
I have to admit that I don't see how any script would make it possible to detect such cases. If it's code from a dependency, there is no way to detect it (and let's not even mention FFI).
Again, I don't see how any script could detect that. I think the only reasonable way to do it is to add an option or a crate doc attribute to say "run all tests in the same thread" which would be disabled by default. And finally: did you compare the before before/after using this new feature? Are the numbers you provided from the current doctests or with the feature enabled? |
No script, I just looked at a hundred or some examples of doc-tests with my eyes. My argument is that it seems to be the more common case, so we should optimize for that and make it the default.
Maybe I'm missing something, but why can't this be a per doc-test attribute?
I tried it out yesterday on master after the feature had been merged and saw no difference for my use-case, but that might be because the bootstrap wasn't updated yet, maybe? |
That doesn't sound like a good metric. ^^'
You would prefer to mark all code examples manually instead?
rustc isn't compiled using the 2024 edition so the feature is not enabled there yet. But again: before trying to build even more options on top of this feature, maybe it'd be better first to check the impact of performance if all doctests were running the same thread. |
It's not great, but better than nothing. We could enable it and do some crater run. I think it's worth a shot.
Not all, only those that need it. Rust is a lot about explicitness, and while for some projects every doctest will need it, I think that's the exception, and for 80+% of cases no attributes are needed at all.
I'm suggesting, multiple threads inside one process. I'm all for giving it a try, how much effort do you guess would such an experiment be for you? I can look into trying it myself, but haven't touched the doctest code at all until now. Starting a process is 20+ million cycles of work usually before main gets even executed, which adds up. |
I ran an approximation of a best-case, where all tests can be run in the same /// This is a demo function, that contains a doc example.
///
/// # Example
///
/// ```
/// let mut vec_example_fn_0 = vec![1, 2, 85590];
/// vec_example_fn_0.push(3);
/// assert_eq!(vec_example_fn_0, [1, 2, 85590, 3]);
/// ```
pub fn example_fn_0() {}
#[test]
fn example_fn_0() {
let mut vec_example_fn_0 = vec![1, 2, 16996];
vec_example_fn_0.push(3);
assert_eq!(vec_example_fn_0, [1, 2, 16996, 3]);
} Results LinuxSetup
edition = "2021"
Breakdown: Clean compile ~1.4s, integration tests ~0.02s, doctests ~16.8s [1] edition = "2024"
Breakdown: Clean compile ~1.4s, integration tests ~0.02s, doctests ~0.1s [1] edition = "2024" and doctest = false
Breakdown: Clean compile ~1.4s, integration tests ~0.02s [1] Results WindowsSetup
edition = "2021"
Breakdown: Clean compile ~2.4s, integration tests ~0.08s, doctests ~51.3s [1] edition = "2024"
Breakdown: Clean compile ~2.4s, integration tests ~0.08s, doctests ~2.3s [1] edition = "2024" and doctest = false
Breakdown: Clean compile ~2.4s, integration tests ~0.08s [1] [1] Numbers as reported by cargo, in reality felt latency can be quite Key takeaways
ReproAll code can be found here: https://github.com/Voultapher/doctest-parallel-experiment |
That overhead seems to be quite low (surprisingly even on Windows). That being said, it would be nice if we could default into running doctests in the same process, especially since it's already the default mode of operation for normal unit tests. Unfortunately, that ship has kind of sailed because doctests were run as separate processes since Rust 1.0. In theory, we could switch the default in Edition 2024, but it might be a too disruptive of a change; it would require people to manually mark problematic tests with an attribute, to opt-out of thread execution. I don't think that we would need a separate attribute though, we could reuse the Since some people actually have problems with running unit tests in threads, maybe a more general solution could be to add some option like |
Good point, Regarding the breaking change, isn't this feature already a breaking change? If so, I think it's reasonably easy to explain that from edition 2024 onwards doctests will be compiled and run the same way as unit and integration tests. My understanding is that the edition mechanism exists exactly to avoid the C++ situation of "yeah we'd like to change that but we can't". Regarding a |
Can we get this discussed by the appropriate team, I think edition 2024 would be the perfect moment to make this change, given we are already breaking doctest compatibility. |
Seems like it might be a bit late in the edition cycle, especially since there's no implementation ready atm. But @traviscross would know the procedure. FWIW, I'm not sure if this is worth it, especially since the perf. benefits seem to be relatively small. I consider it to be a big success that Guillaume managed to speed up doctests so much while keeping the original process semantics. If we also changed the execution behavior, it would make it harder for some projects to make use of the merged compilation mode (and more importantly, harder to switch to the 2024 edition). |
Just like @Kobzol said. I think it could eventually be an option to be enabled, but I'm not sure it's worth the code needed for it. But maybe I'm wrong, a comparison would be needed for that. |
What do you mean by that? |
Well I guess that comes down to personal preference, but for me even having to wait 500ms more during iterative development feels annoying. Usually I just turn off doc-tests because they also make the console output more noisy. |
To measure the impact of using processes vs threads. |
Maybe you missed my comment here #129119 (comment) |
I missed it indeed. |
This is a bit orthogonal, but it's in fact wasteful to compile (and run) all doctests after most recompiles. One alternative to make doctests faster could be to make them incremental and only rebuild/rerun code that has changed. |
It would require to save doctests files but I suppose nothing prevents it. |
How about this path forward. As two separate tasks we tackle:
This way we are not tied to any edition timelines and we can improve the experience users have in an incremental fashion. |
#126245 merged support in Edition 2024 for consolidating doctests into as few binaries as possible, with an opt-out of a
standalone
attribute.I propose by default doc-tests should be run in parallel in the same process.
What's the motivation for the proposed change? For example when working on the Rust standard library, I usually run the tests for
core
andstd/alloc
, that takes incremental ~84s on my Zen 3 5900X machine. Out of that ~66s are spent running the ~4k core doc tests. That's on Linux, on Windows I expect this to be even slower, due to a higher process spawning overhead. Merged doc tests is tied to the Rust 2024 edition because it can break behavior. So there is already precedent for having a new default that is better in the common case, but may break existing projects that opt into the new edition, requiring some work. I believe and doing some code analysis via grep.app would support that the majority of doc tests can be run in parallel in the same binary. So that should be the default. For the cases that depend on some kind of singleton, log lib init, etc. they can be marked with an attribute as requiring a standalone process. That would make for two possible attributes: build isolated and run isolated. If this is deemed too complex, it's imaginable that there is only a single attribute that implies both build and run isolated.Tagging @GuillaumeGomez
The text was updated successfully, but these errors were encountered: